[RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Tiejun Chen]

For the external interrupt, the decrementer exception and the doorbell
excpetion, we also need to soft-disable interrupts while doing as host
interrupt handlers since the DO_KVM hook is always performed to skip
EXCEPTION_COMMON then miss this original chance with the 'ints' (INTS_DISABLE).

Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
---
 arch/powerpc/kvm/bookehv_interrupts.S |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
index e8ed7d6..2fd62bf 100644
--- a/arch/powerpc/kvm/bookehv_interrupts.S
+++ b/arch/powerpc/kvm/bookehv_interrupts.S
@@ -33,6 +33,8 @@
 
 #ifdef CONFIG_64BIT
 #include <asm/exception-64e.h>
+#include <asm/hw_irq.h>
+#include <asm/irqflags.h>
 #else
 #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
 #endif
@@ -469,6 +471,13 @@ _GLOBAL(kvmppc_resume_host)
 	PPC_LL	r3, HOST_RUN(r1)
 	mr	r5, r14 /* intno */
 	mr	r14, r4 /* Save vcpu pointer. */
+#ifdef CONFIG_64BIT
+	/* Should we soft-disable interrupts? */
+	andi.	r6, r5, BOOKE_INTERRUPT_EXTERNAL | BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
+	beq	skip_soft_dis
+	SOFT_DISABLE_INTS(r7,r8)
+skip_soft_dis:
+#endif
 	bl	kvmppc_handle_exit
 
 	/* Restore vcpu pointer and the nonvolatiles we used. */
-- 
1.7.9.5

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/06/2013 11:10 AM, Tiejun Chen wrote:
> For the external interrupt, the decrementer exception and the doorbell
> excpetion, we also need to soft-disable interrupts while doing as host
> interrupt handlers since the DO_KVM hook is always performed to skip
> EXCEPTION_COMMON then miss this original chance with the 'ints' (INTS_DISABLE).

Sorry, miss to send Ben.

Tiejun

>
> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
> ---
>   arch/powerpc/kvm/bookehv_interrupts.S |    9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S b/arch/powerpc/kvm/bookehv_interrupts.S
> index e8ed7d6..2fd62bf 100644
> --- a/arch/powerpc/kvm/bookehv_interrupts.S
> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
> @@ -33,6 +33,8 @@
>
>   #ifdef CONFIG_64BIT
>   #include <asm/exception-64e.h>
> +#include <asm/hw_irq.h>
> +#include <asm/irqflags.h>
>   #else
>   #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
>   #endif
> @@ -469,6 +471,13 @@ _GLOBAL(kvmppc_resume_host)
>   	PPC_LL	r3, HOST_RUN(r1)
>   	mr	r5, r14 /* intno */
>   	mr	r14, r4 /* Save vcpu pointer. */
> +#ifdef CONFIG_64BIT
> +	/* Should we soft-disable interrupts? */
> +	andi.	r6, r5, BOOKE_INTERRUPT_EXTERNAL | BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
> +	beq	skip_soft_dis
> +	SOFT_DISABLE_INTS(r7,r8)
> +skip_soft_dis:
> +#endif
>   	bl	kvmppc_handle_exit
>
>   	/* Restore vcpu pointer and the nonvolatiles we used. */
>

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/07/2013 07:50 AM, Scott Wood wrote:
> On 05/05/2013 10:13:17 PM, tiejun.chen wrote:
>> On 05/06/2013 11:10 AM, Tiejun Chen wrote:
>>> For the external interrupt, the decrementer exception and the doorbell
>>> excpetion, we also need to soft-disable interrupts while doing as host
>>> interrupt handlers since the DO_KVM hook is always performed to skip
>>> EXCEPTION_COMMON then miss this original chance with the 'ints' (INTS_DISABLE).
>
> http://patchwork.ozlabs.org/patch/241344/
> http://patchwork.ozlabs.org/patch/241412/
>
> :-)

I'm observing the same behaviour as well:

	WARN_ON_ONCE(!irqs_disabled());

>
>>> Signed-off-by: Tiejun Chen <tiejun.chen@windriver.com>
>>> ---
>>>   arch/powerpc/kvm/bookehv_interrupts.S |    9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/arch/powerpc/kvm/bookehv_interrupts.S
>>> b/arch/powerpc/kvm/bookehv_interrupts.S
>>> index e8ed7d6..2fd62bf 100644
>>> --- a/arch/powerpc/kvm/bookehv_interrupts.S
>>> +++ b/arch/powerpc/kvm/bookehv_interrupts.S
>>> @@ -33,6 +33,8 @@
>>>
>>>   #ifdef CONFIG_64BIT
>>>   #include <asm/exception-64e.h>
>>> +#include <asm/hw_irq.h>
>>> +#include <asm/irqflags.h>
>>>   #else
>>>   #include "../kernel/head_booke.h" /* for THREAD_NORMSAVE() */
>>>   #endif
>>> @@ -469,6 +471,13 @@ _GLOBAL(kvmppc_resume_host)
>>>       PPC_LL    r3, HOST_RUN(r1)
>>>       mr    r5, r14 /* intno */
>>>       mr    r14, r4 /* Save vcpu pointer. */
>>> +#ifdef CONFIG_64BIT
>>> +    /* Should we soft-disable interrupts? */
>>> +    andi.    r6, r5, BOOKE_INTERRUPT_EXTERNAL | BOOKE_INTERRUPT_DECREMENTER
>>> | BOOKE_INTERRUPT_DOORBELL
>>> +    beq    skip_soft_dis
>>> +    SOFT_DISABLE_INTS(r7,r8)
>>> +skip_soft_dis:
>>> +#endif
>
> Why wouldn't we always disable them?  kvmppc_handle_exit() will enable
> interrupts when it's ready.

This only disable soft interrupt for kvmppc_restart_interrupt() that restarts 
interrupts if they were meant for the host:

a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL | 
BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL

b. bl      kvmppc_handle_exit

c. kvmppc_handle_exit()
{
         int r = RESUME_HOST;
         int s;

         /* update before a new last_exit_type is rewritten */
         kvmppc_update_timing_stats(vcpu);

         /* restart interrupts if they were meant for the host */
         kvmppc_restart_interrupt(vcpu, exit_nr);

         local_irq_enable();	=> Enable again.
....

And shouldn't we handle kvmppc_restart_interrupt() like the original HOST flow?

#define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr, ack)           \
         START_EXCEPTION(label);                                         \
         NORMAL_EXCEPTION_PROLOG(trapnum, intnum, PROLOG_ADDITION_MASKABLE)\
         EXCEPTION_COMMON(trapnum, PACA_EXGEN, *INTS_DISABLE*)             \
	...

So I think this should be reasonable :)

Tiejun

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/07/2013 10:06 AM, Scott Wood wrote:
> On 05/06/2013 08:56:25 PM, tiejun.chen wrote:
>> On 05/07/2013 07:50 AM, Scott Wood wrote:
>>> On 05/05/2013 10:13:17 PM, tiejun.chen wrote:
>>>> On 05/06/2013 11:10 AM, Tiejun Chen wrote:
>>>>> For the external interrupt, the decrementer exception and the doorbell
>>>>> excpetion, we also need to soft-disable interrupts while doing as host
>>>>> interrupt handlers since the DO_KVM hook is always performed to skip
>>>>> EXCEPTION_COMMON then miss this original chance with the 'ints'
>>>>> (INTS_DISABLE).
>>>
>>> http://patchwork.ozlabs.org/patch/241344/
>>> http://patchwork.ozlabs.org/patch/241412/
>>>
>>> :-)
>>
>> I'm observing the same behaviour as well:
>>
>>     WARN_ON_ONCE(!irqs_disabled());
>
> So, could you explain the benefits of your approach over what's being discussed
> in those threads?

They're a long thread so I think I need to take time to see :)

>
>>> Why wouldn't we always disable them?  kvmppc_handle_exit() will enable
>>> interrupts when it's ready.
>>
>> This only disable soft interrupt for kvmppc_restart_interrupt() that restarts
>> interrupts if they were meant for the host:
>>
>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>
> Those aren't the only exceptions that can end up going to the host.  We could
> get a TLB miss that results in a heavyweight MMIO exit, etc.

This is like host handler, so I'm just disabling soft interrupt during 
kvmppc_restart_interrupt() for Doorbell interrupt/Decrementer Interrupt/External 
Input Interrupt.

I don't see anything should be disabled for any TLB exception in host handler.

>
> And I'd rather see any fix for this problem stay out of the asm code.

We already have an appropriate SOFT_DISABLE_INTS so I think we can take this 
easily :)

>
>> b. bl      kvmppc_handle_exit
>>
>> c. kvmppc_handle_exit()
>> {
>>         int r = RESUME_HOST;
>>         int s;
>>
>>         /* update before a new last_exit_type is rewritten */
>>         kvmppc_update_timing_stats(vcpu);
>>
>>         /* restart interrupts if they were meant for the host */
>>         kvmppc_restart_interrupt(vcpu, exit_nr);
>>
>>         local_irq_enable();    => Enable again.
>> ....
>>
>> And shouldn't we handle kvmppc_restart_interrupt() like the original HOST flow?
>>
>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr, ack)           \
>>         START_EXCEPTION(label);                                         \
>>         NORMAL_EXCEPTION_PROLOG(trapnum, intnum, PROLOG_ADDITION_MASKABLE)\
>>         EXCEPTION_COMMON(trapnum, PACA_EXGEN, *INTS_DISABLE*)             \
>>     ...
>
> Could you elaborate on what you mean?

In host handler, we always use MASKABLE_EXCEPTION() to define-to-handle some 
exceptions: Doorbell interrupt/Decrementer Interrupt/External Input Interrupt:

#define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr, ack)           \
         START_EXCEPTION(label);                                         \
         NORMAL_EXCEPTION_PROLOG(trapnum, intnum, PROLOG_ADDITION_MASKABLE)\
         EXCEPTION_COMMON(trapnum, PACA_EXGEN, *INTS_DISABLE*)             \

This would call INTS_DISABLE, which is equal to SOFT_DISABLE_INTS(), to disable 
soft interrupt before call all associated handlers: 
do_IRQ()/timer_interrupt()/doorbell_exception().

But DO_KVM hook always skips INTS_DISABLE.

So I think we also need to do INTS_DISABLE for kvmppc_restart_interrupt() since 
actually that restarts interrupts for the host with a similar way as they are 
called by host.

Tiejun

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/09/2013 03:33 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Linuxppc-dev [mailto:linuxppc-dev-
>> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of Caraman
>> Mihai Claudiu-B02008
>> Sent: Wednesday, May 08, 2013 6:44 PM
>> To: Wood Scott-B07421; tiejun.chen
>> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
>> kvm@vger.kernel.org
>> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
>>
>>>> This only disable soft interrupt for kvmppc_restart_interrupt() that
>>>> restarts interrupts if they were meant for the host:
>>>>
>>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>>>
>>> Those aren't the only exceptions that can end up going to the host.
>>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
>>>
>>>> And shouldn't we handle kvmppc_restart_interrupt() like the original
>>>> HOST flow?
>>>>
>>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
>>>> ack)           \
>>>>
>>>> START_EXCEPTION(label);                                         \
>>>>          NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
>>>> PROLOG_ADDITION_MASKABLE)\
>>>>          EXCEPTION_COMMON(trapnum, PACA_EXGEN,
>>>> *INTS_DISABLE*)             \
>>>> 	...
>>>
>>> Could you elaborate on what you mean?
>>
>> I think Tiejun was saying that host has flags and replays only EE/DEC/DBELL
>> interrupts. There is special macro masked_interrupt_book3e in those exception
>> handlers that sets paca->irq_happened.
>>
>> The list of replied interrupts is limited to asynchronous noncritical interrupts
>> which can be masked by MSR[EE] (therefore no TLB miss).
>
> Embedded Perfmon interrupt is also asynchronous, Why that is not in the list of masked interruts.

Are you saying perfmon? If so, its also in that list:

         START_EXCEPTION(perfmon);
         NORMAL_EXCEPTION_PROLOG(0x260, BOOKE_INTERRUPT_PERFORMANCE_MONITOR,
                                 PROLOG_ADDITION_NONE)
         EXCEPTION_COMMON(0x260, PACA_EXGEN, INTS_DISABLE)

Tiejun

>
> -Bharat
>
>> Now on KVM book3e we
>> don't want to put them in the irq_happened lazy state but rather to execute them
>> directly, so there is no reason for exception handling symmetry between host and
>> guest.
>>
>> -Mike

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/09/2013 03:51 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>> Sent: Thursday, May 09, 2013 1:18 PM
>> To: Bhushan Bharat-R65777
>> Cc: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; linuxppc-
>> dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
>> kvm@vger.kernel.org
>> Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
>>
>> On 05/09/2013 03:33 PM, Bhushan Bharat-R65777 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Linuxppc-dev [mailto:linuxppc-dev-
>>>> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of
>>>> bounces+Caraman
>>>> Mihai Claudiu-B02008
>>>> Sent: Wednesday, May 08, 2013 6:44 PM
>>>> To: Wood Scott-B07421; tiejun.chen
>>>> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de;
>>>> kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
>>>> interrupts
>>>>
>>>>>> This only disable soft interrupt for kvmppc_restart_interrupt()
>>>>>> that restarts interrupts if they were meant for the host:
>>>>>>
>>>>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>>>>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>>>>>
>>>>> Those aren't the only exceptions that can end up going to the host.
>>>>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
>>>>>
>>>>>> And shouldn't we handle kvmppc_restart_interrupt() like the
>>>>>> original HOST flow?
>>>>>>
>>>>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
>>>>>> ack)           \
>>>>>>
>>>>>> START_EXCEPTION(label);                                         \
>>>>>>           NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
>>>>>> PROLOG_ADDITION_MASKABLE)\
>>>>>>           EXCEPTION_COMMON(trapnum, PACA_EXGEN,
>>>>>> *INTS_DISABLE*)             \
>>>>>> 	...
>>>>>
>>>>> Could you elaborate on what you mean?
>>>>
>>>> I think Tiejun was saying that host has flags and replays only
>>>> EE/DEC/DBELL interrupts. There is special macro
>>>> masked_interrupt_book3e in those exception handlers that sets paca-
>>> irq_happened.
>>>>
>>>> The list of replied interrupts is limited to asynchronous noncritical
>>>> interrupts which can be masked by MSR[EE] (therefore no TLB miss).
>>>
>>> Embedded Perfmon interrupt is also asynchronous, Why that is not in the list
>> of masked interruts.
>>
>> Are you saying perfmon? If so, its also in that list:
>>
>>           START_EXCEPTION(perfmon);
>>           NORMAL_EXCEPTION_PROLOG(0x260, BOOKE_INTERRUPT_PERFORMANCE_MONITOR,
>>                                   PROLOG_ADDITION_NONE)
>>           EXCEPTION_COMMON(0x260, PACA_EXGEN, INTS_DISABLE)
>
> Where it is recorded in paca->irq_happned to be replayed later ?

In stead of PROLOG_ADDITION_MASKABLE, actually we have nothing to do with 
PROLOG_ADDITION_NONE for perfmon, so perfmon can be executed without lazy state.

Tiejun

>
>>
>> Tiejun
>>
>>>
>>> -Bharat
>>>
>>>> Now on KVM book3e we
>>>> don't want to put them in the irq_happened lazy state but rather to
>>>> execute them directly, so there is no reason for exception handling
>>>> symmetry between host and guest.
>>>>
>>>> -Mike
>>
>

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 4122 bytes --]

On Thu, May 09, 2013 at 07:51:09AM +0000, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: tiejun.chen [mailto:tiejun.chen@windriver.com]
> > Sent: Thursday, May 09, 2013 1:18 PM
> > To: Bhushan Bharat-R65777
> > Cc: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; linuxppc-
> > dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
> > kvm@vger.kernel.org
> > Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
> > 
> > On 05/09/2013 03:33 PM, Bhushan Bharat-R65777 wrote:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Linuxppc-dev [mailto:linuxppc-dev-
> > >> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of
> > >> bounces+Caraman
> > >> Mihai Claudiu-B02008
> > >> Sent: Wednesday, May 08, 2013 6:44 PM
> > >> To: Wood Scott-B07421; tiejun.chen
> > >> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de;
> > >> kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> > >> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
> > >> interrupts
> > >>
> > >>>> This only disable soft interrupt for kvmppc_restart_interrupt()
> > >>>> that restarts interrupts if they were meant for the host:
> > >>>>
> > >>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
> > >>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
> > >>>
> > >>> Those aren't the only exceptions that can end up going to the host.
> > >>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
> > >>>
> > >>>> And shouldn't we handle kvmppc_restart_interrupt() like the
> > >>>> original HOST flow?
> > >>>>
> > >>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
> > >>>> ack)           \
> > >>>>
> > >>>> START_EXCEPTION(label);                                         \
> > >>>>          NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
> > >>>> PROLOG_ADDITION_MASKABLE)\
> > >>>>          EXCEPTION_COMMON(trapnum, PACA_EXGEN,
> > >>>> *INTS_DISABLE*)             \
> > >>>> 	...
> > >>>
> > >>> Could you elaborate on what you mean?
> > >>
> > >> I think Tiejun was saying that host has flags and replays only
> > >> EE/DEC/DBELL interrupts. There is special macro
> > >> masked_interrupt_book3e in those exception handlers that sets paca-
> > >irq_happened.
> > >>
> > >> The list of replied interrupts is limited to asynchronous noncritical
> > >> interrupts which can be masked by MSR[EE] (therefore no TLB miss).
> > >
> > > Embedded Perfmon interrupt is also asynchronous, Why that is not in the list
> > of masked interruts.
> > 
> > Are you saying perfmon? If so, its also in that list:
> > 
> >          START_EXCEPTION(perfmon);
> >          NORMAL_EXCEPTION_PROLOG(0x260, BOOKE_INTERRUPT_PERFORMANCE_MONITOR,
> >                                  PROLOG_ADDITION_NONE)
> >          EXCEPTION_COMMON(0x260, PACA_EXGEN, INTS_DISABLE)
> 
> Where it is recorded in paca->irq_happned to be replayed later ?

Actually we don't want replay the perfmon interrupt later. We would run it
even soft irq is disabled and just treat it as NMI. Please see the following
function quoted from arch/powerpc/perf/core-fsl-emb.c:
  /*
   * If interrupts were soft-disabled when a PMU interrupt occurs, treat
   * it as an NMI.
   */
  static inline int perf_intr_is_nmi(struct pt_regs *regs)
  {
  #ifdef __powerpc64__                                                                                                                                                         
          return !regs->softe;
  #else
          return 0;
  #endif
  }

Thanks,
Kevin

> 
> > 
> > Tiejun
> > 
> > >
> > > -Bharat
> > >
> > >> Now on KVM book3e we
> > >> don't want to put them in the irq_happened lazy state but rather to
> > >> execute them directly, so there is no reason for exception handling
> > >> symmetry between host and guest.
> > >>
> > >> -Mike
> > 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/09/2013 04:12 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Kevin Hao [mailto:haokexin@gmail.com]
>> Sent: Thursday, May 09, 2013 1:38 PM
>> To: Bhushan Bharat-R65777
>> Cc: tiejun.chen; Caraman Mihai Claudiu-B02008; kvm@vger.kernel.org; Wood Scott-
>> B07421; agraf@suse.de; kvm-ppc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
>> Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
>>
>> On Thu, May 09, 2013 at 07:51:09AM +0000, Bhushan Bharat-R65777 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>>>> Sent: Thursday, May 09, 2013 1:18 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; linuxppc-
>>>> dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
>>>> kvm@vger.kernel.org
>>>> Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
>>>> interrupts
>>>>
>>>> On 05/09/2013 03:33 PM, Bhushan Bharat-R65777 wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Linuxppc-dev [mailto:linuxppc-dev-
>>>>>> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf
>>>>>> bounces+Of Caraman
>>>>>> Mihai Claudiu-B02008
>>>>>> Sent: Wednesday, May 08, 2013 6:44 PM
>>>>>> To: Wood Scott-B07421; tiejun.chen
>>>>>> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de;
>>>>>> kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>>>> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
>>>>>> interrupts
>>>>>>
>>>>>>>> This only disable soft interrupt for kvmppc_restart_interrupt()
>>>>>>>> that restarts interrupts if they were meant for the host:
>>>>>>>>
>>>>>>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>>>>>>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>>>>>>>
>>>>>>> Those aren't the only exceptions that can end up going to the host.
>>>>>>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
>>>>>>>
>>>>>>>> And shouldn't we handle kvmppc_restart_interrupt() like the
>>>>>>>> original HOST flow?
>>>>>>>>
>>>>>>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
>>>>>>>> ack)           \
>>>>>>>>
>>>>>>>> START_EXCEPTION(label);                                         \
>>>>>>>>           NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
>>>>>>>> PROLOG_ADDITION_MASKABLE)\
>>>>>>>>           EXCEPTION_COMMON(trapnum, PACA_EXGEN,
>>>>>>>> *INTS_DISABLE*)             \
>>>>>>>> 	...
>>>>>>>
>>>>>>> Could you elaborate on what you mean?
>>>>>>
>>>>>> I think Tiejun was saying that host has flags and replays only
>>>>>> EE/DEC/DBELL interrupts. There is special macro
>>>>>> masked_interrupt_book3e in those exception handlers that sets
>>>>>> paca-
>>>>> irq_happened.
>>>>>>
>>>>>> The list of replied interrupts is limited to asynchronous
>>>>>> noncritical interrupts which can be masked by MSR[EE] (therefore no TLB
>> miss).
>>>>>
>>>>> Embedded Perfmon interrupt is also asynchronous, Why that is not
>>>>> in the list
>>>> of masked interruts.
>>>>
>>>> Are you saying perfmon? If so, its also in that list:
>>>>
>>>>           START_EXCEPTION(perfmon);
>>>>           NORMAL_EXCEPTION_PROLOG(0x260, BOOKE_INTERRUPT_PERFORMANCE_MONITOR,
>>>>                                   PROLOG_ADDITION_NONE)
>>>>           EXCEPTION_COMMON(0x260, PACA_EXGEN, INTS_DISABLE)
>>>
>>> Where it is recorded in paca->irq_happned to be replayed later ?
>>
>> Actually we don't want replay the perfmon interrupt later. We would run it even
>> soft irq is disabled and just treat it as NMI. Please see the following function
>> quoted from arch/powerpc/perf/core-fsl-emb.c:
>>    /*
>>     * If interrupts were soft-disabled when a PMU interrupt occurs, treat
>>     * it as an NMI.
>>     */
>>    static inline int perf_intr_is_nmi(struct pt_regs *regs)
>>    {
>>    #ifdef __powerpc64__
>>            return !regs->softe;
>>    #else
>>            return 0;
>>    #endif
>>    }
>
> Is it because that we cannot afford to lose perfmon interrupt for more accurate capturing of data ?

     powerpc/perf: e500 support

     This implements perf_event support for the Freescale embedded performance
     monitor, based on the existing perf_event.c that supports server/classic
     chips.

     Some limitations:
     - Performance monitor interrupts are regular EE interrupts, and thus you
       can't profile places with interrupts disabled.  We may want to implement
       soft IRQ-disabling, with perfmon interrupts exempted and treated as NMIs.

Tiejun

>
> -Bharat
>
>>
>> Thanks,
>> Kevin
>>
>>>
>>>>
>>>> Tiejun
>>>>
>>>>>
>>>>> -Bharat
>>>>>
>>>>>> Now on KVM book3e we
>>>>>> don't want to put them in the irq_happened lazy state but rather
>>>>>> to execute them directly, so there is no reason for exception
>>>>>> handling symmetry between host and guest.
>>>>>>
>>>>>> -Mike
>>>>
>>>
>>> _______________________________________________
>>> Linuxppc-dev mailing list
>>> Linuxppc-dev@lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>
>
>

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 5141 bytes --]

On Thu, May 09, 2013 at 08:12:51AM +0000, Bhushan Bharat-R65777 wrote:
> 
> 
> > -----Original Message-----
> > From: Kevin Hao [mailto:haokexin@gmail.com]
> > Sent: Thursday, May 09, 2013 1:38 PM
> > To: Bhushan Bharat-R65777
> > Cc: tiejun.chen; Caraman Mihai Claudiu-B02008; kvm@vger.kernel.org; Wood Scott-
> > B07421; agraf@suse.de; kvm-ppc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org
> > Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
> > 
> > On Thu, May 09, 2013 at 07:51:09AM +0000, Bhushan Bharat-R65777 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: tiejun.chen [mailto:tiejun.chen@windriver.com]
> > > > Sent: Thursday, May 09, 2013 1:18 PM
> > > > To: Bhushan Bharat-R65777
> > > > Cc: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; linuxppc-
> > > > dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
> > > > kvm@vger.kernel.org
> > > > Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
> > > > interrupts
> > > >
> > > > On 05/09/2013 03:33 PM, Bhushan Bharat-R65777 wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Linuxppc-dev [mailto:linuxppc-dev-
> > > > >> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf
> > > > >> bounces+Of Caraman
> > > > >> Mihai Claudiu-B02008
> > > > >> Sent: Wednesday, May 08, 2013 6:44 PM
> > > > >> To: Wood Scott-B07421; tiejun.chen
> > > > >> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de;
> > > > >> kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> > > > >> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
> > > > >> interrupts
> > > > >>
> > > > >>>> This only disable soft interrupt for kvmppc_restart_interrupt()
> > > > >>>> that restarts interrupts if they were meant for the host:
> > > > >>>>
> > > > >>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
> > > > >>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
> > > > >>>
> > > > >>> Those aren't the only exceptions that can end up going to the host.
> > > > >>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
> > > > >>>
> > > > >>>> And shouldn't we handle kvmppc_restart_interrupt() like the
> > > > >>>> original HOST flow?
> > > > >>>>
> > > > >>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
> > > > >>>> ack)           \
> > > > >>>>
> > > > >>>> START_EXCEPTION(label);                                         \
> > > > >>>>          NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
> > > > >>>> PROLOG_ADDITION_MASKABLE)\
> > > > >>>>          EXCEPTION_COMMON(trapnum, PACA_EXGEN,
> > > > >>>> *INTS_DISABLE*)             \
> > > > >>>> 	...
> > > > >>>
> > > > >>> Could you elaborate on what you mean?
> > > > >>
> > > > >> I think Tiejun was saying that host has flags and replays only
> > > > >> EE/DEC/DBELL interrupts. There is special macro
> > > > >> masked_interrupt_book3e in those exception handlers that sets
> > > > >> paca-
> > > > >irq_happened.
> > > > >>
> > > > >> The list of replied interrupts is limited to asynchronous
> > > > >> noncritical interrupts which can be masked by MSR[EE] (therefore no TLB
> > miss).
> > > > >
> > > > > Embedded Perfmon interrupt is also asynchronous, Why that is not
> > > > > in the list
> > > > of masked interruts.
> > > >
> > > > Are you saying perfmon? If so, its also in that list:
> > > >
> > > >          START_EXCEPTION(perfmon);
> > > >          NORMAL_EXCEPTION_PROLOG(0x260, BOOKE_INTERRUPT_PERFORMANCE_MONITOR,
> > > >                                  PROLOG_ADDITION_NONE)
> > > >          EXCEPTION_COMMON(0x260, PACA_EXGEN, INTS_DISABLE)
> > >
> > > Where it is recorded in paca->irq_happned to be replayed later ?
> > 
> > Actually we don't want replay the perfmon interrupt later. We would run it even
> > soft irq is disabled and just treat it as NMI. Please see the following function
> > quoted from arch/powerpc/perf/core-fsl-emb.c:
> >   /*
> >    * If interrupts were soft-disabled when a PMU interrupt occurs, treat
> >    * it as an NMI.
> >    */
> >   static inline int perf_intr_is_nmi(struct pt_regs *regs)
> >   {
> >   #ifdef __powerpc64__
> >           return !regs->softe;
> >   #else
> >           return 0;
> >   #endif
> >   }
> 
> Is it because that we cannot afford to lose perfmon interrupt for more accurate capturing of data ?

Yes, I think this will definitely improve the perf sample quality.

Thanks,
Kevin

> 
> -Bharat
> 
> > 
> > Thanks,
> > Kevin
> > 
> > >
> > > >
> > > > Tiejun
> > > >
> > > > >
> > > > > -Bharat
> > > > >
> > > > >> Now on KVM book3e we
> > > > >> don't want to put them in the irq_happened lazy state but rather
> > > > >> to execute them directly, so there is no reason for exception
> > > > >> handling symmetry between host and guest.
> > > > >>
> > > > >> -Mike
> > > >
> > >
> > > _______________________________________________
> > > Linuxppc-dev mailing list
> > > Linuxppc-dev@lists.ozlabs.org
> > > https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Benjamin Herrenschmidt]

On Thu, 2013-05-09 at 16:21 +0800, Kevin Hao wrote:
> > Is it because that we cannot afford to lose perfmon interrupt for
> more accurate capturing of data ?
> 
> Yes, I think this will definitely improve the perf sample quality.

This is one of the primary reason why we implemented lazy disabling in
the first place and why I recently reworked it to decrease the periods
where we are hard disabled.

The other reasons are that storing bytes to the PACA is faster than
manipulating EE on many processors.

Cheers,
Ben.

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/09/2013 04:23 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: Linuxppc-dev [mailto:linuxppc-dev-
>> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of Caraman
>> Mihai Claudiu-B02008
>> Sent: Wednesday, May 08, 2013 6:44 PM
>> To: Wood Scott-B07421; tiejun.chen
>> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
>> kvm@vger.kernel.org
>> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
>>
>>>> This only disable soft interrupt for kvmppc_restart_interrupt() that
>>>> restarts interrupts if they were meant for the host:
>>>>
>>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>>>
>>> Those aren't the only exceptions that can end up going to the host.
>>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
>>>
>>>> And shouldn't we handle kvmppc_restart_interrupt() like the original
>>>> HOST flow?
>>>>
>>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
>>>> ack)           \
>>>>
>>>> START_EXCEPTION(label);                                         \
>>>>          NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
>>>> PROLOG_ADDITION_MASKABLE)\
>>>>          EXCEPTION_COMMON(trapnum, PACA_EXGEN,
>>>> *INTS_DISABLE*)             \
>>>> 	...
>>>
>>> Could you elaborate on what you mean?
>>
>> I think Tiejun was saying that host has flags and replays only EE/DEC/DBELL
>> interrupts. There is special macro masked_interrupt_book3e in those exception
>> handlers that sets paca->irq_happened.
>>
>> The list of replied interrupts is limited to asynchronous noncritical interrupts
>> which can be masked by MSR[EE] (therefore no TLB miss). Now on KVM book3e we
>> don't want to put them in the irq_happened lazy state but rather to execute them
>> directly, so there is no reason for exception handling symmetry between host and
>> guest.
>
>
> Another Question:
>
> The case is:
>

Actually in the case GS=1 even if EE=0, EXT/DEC/DBELL still occur as I recall.

> Case 1)
>   -> Local_irq_disable()  will set soft_enabled = 0
>   -> Now Externel interrupt happens, there we set PACA_IRQ_EE in irq_happened, Also clears EE in SRR1 and rfi. So interrupts are hard disabled. No more other interrupt gated by MSR.EE can happen. Looks like the idea here is to not let a device keep on inserting interrupt till the interrupt condition on device is cleared, right?

I don't understand "the interrupt condition on device is cleared" here.

I think regardless if you clear the device interrupt status, the system still 
receive a pending interrupt once EE or GS = 1.

>   -> local_irq_enable() - This checks that irq_happened is set, and replays

ret_from_except also check to replay.

>
> Now the case 2)
> Case 2)
> -> Local_irq_disable()  will set soft_enabled = 0
>   -> Now DEC interrupt happens. We set PACA_IRQ_DEC in irq_happened, But do not clear EE in SRR1 and rfi. So interrupts are not hard disabled.
>   -> Now say EE interrupt happens, there we set PACA_IRQ_EE in irq_happened, Also clears EE in SRR1 and rfi. So interrupts are hard disabled.
>   -> local_irq_enable() - This checks that irq_happened is set.
> IIUC, it replays only one interrupt? is not it?

After anyone is replayed in arch_local_irq_restore(), we will set soft/hard 
interrupt there:

set_soft_enabled(1);
__hard_irq_enable();

Then any pending interrupt can be executed now.

Additionally, ret_from_except probably check to replay all.

Tiejun

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/09/2013 06:00 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>> Sent: Thursday, May 09, 2013 3:15 PM
>> To: Bhushan Bharat-R65777
>> Cc: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; linuxppc-
>> dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
>> kvm@vger.kernel.org
>> Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
>>
>> On 05/09/2013 04:23 PM, Bhushan Bharat-R65777 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Linuxppc-dev [mailto:linuxppc-dev-
>>>> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of
>>>> bounces+Caraman
>>>> Mihai Claudiu-B02008
>>>> Sent: Wednesday, May 08, 2013 6:44 PM
>>>> To: Wood Scott-B07421; tiejun.chen
>>>> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de;
>>>> kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
>>>> interrupts
>>>>
>>>>>> This only disable soft interrupt for kvmppc_restart_interrupt()
>>>>>> that restarts interrupts if they were meant for the host:
>>>>>>
>>>>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>>>>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>>>>>
>>>>> Those aren't the only exceptions that can end up going to the host.
>>>>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
>>>>>
>>>>>> And shouldn't we handle kvmppc_restart_interrupt() like the
>>>>>> original HOST flow?
>>>>>>
>>>>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
>>>>>> ack)           \
>>>>>>
>>>>>> START_EXCEPTION(label);                                         \
>>>>>>           NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
>>>>>> PROLOG_ADDITION_MASKABLE)\
>>>>>>           EXCEPTION_COMMON(trapnum, PACA_EXGEN,
>>>>>> *INTS_DISABLE*)             \
>>>>>> 	...
>>>>>
>>>>> Could you elaborate on what you mean?
>>>>
>>>> I think Tiejun was saying that host has flags and replays only
>>>> EE/DEC/DBELL interrupts. There is special macro
>>>> masked_interrupt_book3e in those exception handlers that sets paca-
>>> irq_happened.
>>>>
>>>> The list of replied interrupts is limited to asynchronous noncritical
>>>> interrupts which can be masked by MSR[EE] (therefore no TLB miss).
>>>> Now on KVM book3e we don't want to put them in the irq_happened lazy
>>>> state but rather to execute them directly, so there is no reason for
>>>> exception handling symmetry between host and guest.
>>>
>>>
>>> Another Question:
>>>
>>> The case is:
>>>
>>
>> Actually in the case GS=1 even if EE=0, EXT/DEC/DBELL still occur as I recall.
>>
>>> Case 1)
>>>    -> Local_irq_disable()  will set soft_enabled = 0
>>>    -> Now Externel interrupt happens, there we set PACA_IRQ_EE in irq_happened,
>> Also clears EE in SRR1 and rfi. So interrupts are hard disabled. No more other
>> interrupt gated by MSR.EE can happen. Looks like the idea here is to not let a
>> device keep on inserting interrupt till the interrupt condition on device is
>> cleared, right?
>>
>> I don't understand "the interrupt condition on device is cleared" here.
>>
>> I think regardless if you clear the device interrupt status, the system still
>> receive a pending interrupt once EE or GS = 1.
>
> Once yes, but I think to avoid flood of device interrupt we disable MSR.EE when soft-disabled.

But we neither ACK nor send EOI to that irq in the interrupt controller, so that 
should be in pending state.

>
>>
>>>    -> local_irq_enable() - This checks that irq_happened is set, and
>>> replays
>>
>> ret_from_except also check to replay.
>>
>>>
>>> Now the case 2)
>>> Case 2)
>>> -> Local_irq_disable()  will set soft_enabled = 0
>>>    -> Now DEC interrupt happens. We set PACA_IRQ_DEC in irq_happened, But do
>> not clear EE in SRR1 and rfi. So interrupts are not hard disabled.
>>>    -> Now say EE interrupt happens, there we set PACA_IRQ_EE in irq_happened,
>> Also clears EE in SRR1 and rfi. So interrupts are hard disabled.
>>>    -> local_irq_enable() - This checks that irq_happened is set.
>>> IIUC, it replays only one interrupt? is not it?
>>
>> After anyone is replayed in arch_local_irq_restore(), we will set soft/hard
>> interrupt there:
>>
>> set_soft_enabled(1);
>> __hard_irq_enable();
>>
>> Then any pending interrupt can be executed now.
>
> Do you mean that the interrupt should fire again?

I means the pending exception including external interrupt, the decrementer 
exception and the doorbell exception, can trap CPU once EE=1 with 
__hard_irq_enable() here. Then the kernel can handle those exception since soft 
enable is also 1 now.

>
>>
>> Additionally, ret_from_except probably check to replay all.
>
> Local_irq_enable() will not take us to ret_from_except.

Yes. I just say ret_from_except can provide an approach to replay all :)

Tiejun

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[tiejun.chen]

On 05/09/2013 07:21 PM, Bhushan Bharat-R65777 wrote:
>
>
>> -----Original Message-----
>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>> Sent: Thursday, May 09, 2013 3:48 PM
>> To: Bhushan Bharat-R65777
>> Cc: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; linuxppc-
>> dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
>> kvm@vger.kernel.org
>> Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts
>>
>> On 05/09/2013 06:00 PM, Bhushan Bharat-R65777 wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: tiejun.chen [mailto:tiejun.chen@windriver.com]
>>>> Sent: Thursday, May 09, 2013 3:15 PM
>>>> To: Bhushan Bharat-R65777
>>>> Cc: Caraman Mihai Claudiu-B02008; Wood Scott-B07421; linuxppc-
>>>> dev@lists.ozlabs.org; agraf@suse.de; kvm-ppc@vger.kernel.org;
>>>> kvm@vger.kernel.org
>>>> Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
>>>> interrupts
>>>>
>>>> On 05/09/2013 04:23 PM, Bhushan Bharat-R65777 wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Linuxppc-dev [mailto:linuxppc-dev-
>>>>>> bounces+bharat.bhushan=freescale.com@lists.ozlabs.org] On Behalf Of
>>>>>> bounces+Caraman
>>>>>> Mihai Claudiu-B02008
>>>>>> Sent: Wednesday, May 08, 2013 6:44 PM
>>>>>> To: Wood Scott-B07421; tiejun.chen
>>>>>> Cc: linuxppc-dev@lists.ozlabs.org; agraf@suse.de;
>>>>>> kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
>>>>>> Subject: RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable
>>>>>> interrupts
>>>>>>
>>>>>>>> This only disable soft interrupt for kvmppc_restart_interrupt()
>>>>>>>> that restarts interrupts if they were meant for the host:
>>>>>>>>
>>>>>>>> a. SOFT_DISABLE_INTS() only for BOOKE_INTERRUPT_EXTERNAL |
>>>>>>>> BOOKE_INTERRUPT_DECREMENTER | BOOKE_INTERRUPT_DOORBELL
>>>>>>>
>>>>>>> Those aren't the only exceptions that can end up going to the host.
>>>>>>> We could get a TLB miss that results in a heavyweight MMIO exit, etc.
>>>>>>>
>>>>>>>> And shouldn't we handle kvmppc_restart_interrupt() like the
>>>>>>>> original HOST flow?
>>>>>>>>
>>>>>>>> #define MASKABLE_EXCEPTION(trapnum, intnum, label, hdlr,
>>>>>>>> ack)           \
>>>>>>>>
>>>>>>>> START_EXCEPTION(label);                                         \
>>>>>>>>            NORMAL_EXCEPTION_PROLOG(trapnum, intnum,
>>>>>>>> PROLOG_ADDITION_MASKABLE)\
>>>>>>>>            EXCEPTION_COMMON(trapnum, PACA_EXGEN,
>>>>>>>> *INTS_DISABLE*)             \
>>>>>>>> 	...
>>>>>>>
>>>>>>> Could you elaborate on what you mean?
>>>>>>
>>>>>> I think Tiejun was saying that host has flags and replays only
>>>>>> EE/DEC/DBELL interrupts. There is special macro
>>>>>> masked_interrupt_book3e in those exception handlers that sets paca-
>>>>> irq_happened.
>>>>>>
>>>>>> The list of replied interrupts is limited to asynchronous
>>>>>> noncritical interrupts which can be masked by MSR[EE] (therefore no TLB
>> miss).
>>>>>> Now on KVM book3e we don't want to put them in the irq_happened
>>>>>> lazy state but rather to execute them directly, so there is no
>>>>>> reason for exception handling symmetry between host and guest.
>>>>>
>>>>>
>>>>> Another Question:
>>>>>
>>>>> The case is:
>>>>>
>>>>
>>>> Actually in the case GS=1 even if EE=0, EXT/DEC/DBELL still occur as I
>> recall.
>>>>
>>>>> Case 1)
>>>>>     -> Local_irq_disable()  will set soft_enabled = 0
>>>>>     -> Now Externel interrupt happens, there we set PACA_IRQ_EE in
>>>>> irq_happened,
>>>> Also clears EE in SRR1 and rfi. So interrupts are hard disabled. No
>>>> more other interrupt gated by MSR.EE can happen. Looks like the idea
>>>> here is to not let a device keep on inserting interrupt till the
>>>> interrupt condition on device is cleared, right?
>>>>
>>>> I don't understand "the interrupt condition on device is cleared" here.
>>>>
>>>> I think regardless if you clear the device interrupt status, the
>>>> system still receive a pending interrupt once EE or GS = 1.
>>>
>>> Once yes, but I think to avoid flood of device interrupt we disable MSR.EE
>> when soft-disabled.
>>
>> But we neither ACK nor send EOI to that irq in the interrupt controller, so that
>> should be in pending state.
>>
>>>
>>>>
>>>>>     -> local_irq_enable() - This checks that irq_happened is set, and
>>>>> replays
>>>>
>>>> ret_from_except also check to replay.
>>>>
>>>>>
>>>>> Now the case 2)
>>>>> Case 2)
>>>>> -> Local_irq_disable()  will set soft_enabled = 0
>>>>>     -> Now DEC interrupt happens. We set PACA_IRQ_DEC in
>>>>> irq_happened, But do
>>>> not clear EE in SRR1 and rfi. So interrupts are not hard disabled.
>>>>>     -> Now say EE interrupt happens, there we set PACA_IRQ_EE in
>>>>> irq_happened,
>>>> Also clears EE in SRR1 and rfi. So interrupts are hard disabled.
>>>>>     -> local_irq_enable() - This checks that irq_happened is set.
>>>>> IIUC, it replays only one interrupt? is not it?
>>>>
>>>> After anyone is replayed in arch_local_irq_restore(), we will set
>>>> soft/hard interrupt there:
>>>>
>>>> set_soft_enabled(1);
>>>> __hard_irq_enable();
>>>>
>>>> Then any pending interrupt can be executed now.
>>>
>>> Do you mean that the interrupt should fire again?
>>
>> I means the pending exception including external interrupt, the decrementer
>> exception and the doorbell exception, can trap CPU once EE=1 with
>> __hard_irq_enable() here. Then the kernel can handle those exception since soft
>> enable is also 1 now.
>>
>>>
>>>>
>>>> Additionally, ret_from_except probably check to replay all.
>>>
>>> Local_irq_enable() will not take us to ret_from_except.
>>
>> Yes. I just say ret_from_except can provide an approach to replay all :)
>
> __replay_interrupt() from arch_local_irq_enable() will take us to ret_from_except/lite :)
> There all pending interrupts are replayed one by one before we hard-enable and soft-enable interrupts.

Yes, but a point needs to be corrected,

_replay_interrupt() is following set_soft_enabled(1), so __replay_interrupt() 
can go the exception entry to call the handler.

Tiejun

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Benjamin Herrenschmidt]

On Thu, 2013-05-09 at 17:44 +0800, tiejun.chen wrote:
> 
> Actually in the case GS=1 even if EE=0, EXT/DEC/DBELL still occur as I
> recall.

Only if directed to the hypervisor.

> > Case 1)
> >   -> Local_irq_disable()  will set soft_enabled = 0
> >   -> Now Externel interrupt happens, there we set PACA_IRQ_EE in
> irq_happened, Also clears EE in SRR1 and rfi. So interrupts are hard
> disabled. No more other interrupt gated by MSR.EE can happen. Looks
> like the idea here is to not let a device keep on inserting interrupt
> till the interrupt condition on device is cleared, right?

The external interrupt line is level sensitive normally, so we have to
mask MSR:EE in that case or the interrupt would keep re-occurring (note
that FSL has this concept of auto-acked interrupts via the on die MPIC
for which you can potentially use PACA_IRQ_EE_EDGE instead and avoid
having to mask MSR:EE).

> I don't understand "the interrupt condition on device is cleared"
> here.

Well, the external interrupt line will remain asserted until the device
(via the PIC) stops interrupting the processor :-) Either because we
mask at the PIC level (or raise the priority) or because the condition
goes away.
 
> I think regardless if you clear the device interrupt status, the
> system still  receive a pending interrupt once EE or GS = 1.

Why ? Depends on your PIC actually but if it's a sane one, it won't
"remember" a level interrupt that is gone. An edge interrupt is a
different matter and is latched.

Some MPIC implementations tend to generate a spurrious IRQ in the case
of level IRQs going away. IE. they still remember an event occurred and
interrupt the processor, but on IACK return the spurious vector. However
that isn't guaranteed to be the case and it is perfectly ok (and a good
idea) for the PIC to just stop asserting the external interrupt line if
the original device interrupt goes away (and it's configured as level
sensitive).
 
You might notice that we try to re-hard-enable (while still soft
disabled) as soon as we have gone get_irq in do_IRQ. This is because
fetching the interrupt normally also masks it (either by masking at the
source or by raisin the processor interrupt priority depending on the
specific PIC) so we know we can re-hard enable. 

> >   -> local_irq_enable() - This checks that irq_happened is set, and
> replays
> 
> ret_from_except also check to replay.

ret_from_except checks because it essentially can act as an implicit
local_irq_enable.

> > Now the case 2)
> > Case 2)
> > -> Local_irq_disable()  will set soft_enabled = 0
> >   -> Now DEC interrupt happens. We set PACA_IRQ_DEC in irq_happened,
> But do not clear EE in SRR1 and rfi. So interrupts are not hard
> disabled.

Right. We move the decrementer to its maximum value however since on
most implementations, it will continue interrupting the processor while
it's top bit is set (and effectively act as a level sensitive
interrupt).

Again, the goal here is to run as much as possible with interrupts hard
enabled which allow better perf sampling.

> >   -> Now say EE interrupt happens, there we set PACA_IRQ_EE in
> irq_happened, Also clears EE in SRR1 and rfi. So interrupts are hard
> disabled.
> >   -> local_irq_enable() - This checks that irq_happened is set.
> > IIUC, it replays only one interrupt? is not it?

It replays one interrupt, but interrupt are still disabled during the
replay. Upon return from the replayed interrupt, the ret_from_except
will see the other one and replay it too.

> After anyone is replayed in arch_local_irq_restore(), we will set
> soft/hard 
> interrupt there:
> 
> set_soft_enabled(1);
> __hard_irq_enable();
> 
> Then any pending interrupt can be executed now.
> 
> Additionally, ret_from_except probably check to replay all.
> 
> Tiejun
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[David Laight]

> Some MPIC implementations tend to generate a spurrious IRQ in the case
> of level IRQs going away. IE. they still remember an event occurred and
> interrupt the processor, but on IACK return the spurious vector. However
> that isn't guaranteed to be the case and it is perfectly ok (and a good
> idea) for the PIC to just stop asserting the external interrupt line if
> the original device interrupt goes away (and it's configured as level
> sensitive).

That will happen if the IRQ goes away while the cpu is performing
the IACK sequence.
If the IRQ goes away while the cpu has interrupts masked then
the cpu won't start the interrupt sequence and then try to
read a vector when no interrupt is pending.

	David

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Benjamin Herrenschmidt]

On Thu, 2013-05-09 at 14:28 +0100, David Laight wrote:
> That will happen if the IRQ goes away while the cpu is performing
> the IACK sequence.
> If the IRQ goes away while the cpu has interrupts masked then
> the cpu won't start the interrupt sequence and then try to
> read a vector when no interrupt is pending.

Right, and get a spurrious vector which shouldn't be a big deal.

We tend to call that a "short interrupt". There have been many other
cases of short interrupts in the past, for example on some MPICs, when
distribution is enabled, they occasionally shoot an interrupt to more
than one CPU at once :-)

Cheers,
Ben.

RE: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Chen, Tiejun]

> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] 
> Sent: Thursday, May 09, 2013 8:38 PM
> To: Chen, Tiejun
> Cc: Bhushan Bharat-R65777; Caraman Mihai Claudiu-B02008; Wood 
> Scott-B07421; linuxppc-dev@lists.ozlabs.org; agraf@suse.de; 
> kvm-ppc@vger.kernel.org; kvm@vger.kernel.org
> Subject: Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: 
> soft-disable interrupts
> 
> On Thu, 2013-05-09 at 17:44 +0800, tiejun.chen wrote:
> > 
> > Actually in the case GS=1 even if EE=0, EXT/DEC/DBELL still 
> occur as I 
> > recall.
> 
> Only if directed to the hypervisor.

Yes, this is our current booehv design with EPCR[EXTGS] = 0.

> 
> > > Case 1)
> > >   -> Local_irq_disable()  will set soft_enabled = 0
> > >   -> Now Externel interrupt happens, there we set PACA_IRQ_EE in
> > irq_happened, Also clears EE in SRR1 and rfi. So interrupts 
> are hard 
> > disabled. No more other interrupt gated by MSR.EE can happen. Looks 
> > like the idea here is to not let a device keep on inserting 
> interrupt 
> > till the interrupt condition on device is cleared, right?
> 
> The external interrupt line is level sensitive normally, so 
> we have to mask MSR:EE in that case or the interrupt would 
> keep re-occurring (note that FSL has this concept of 
> auto-acked interrupts via the on die MPIC for which you can 
> potentially use PACA_IRQ_EE_EDGE instead and avoid having to 
> mask MSR:EE).
> 
> > I don't understand "the interrupt condition on device is cleared"
> > here.
> 
> Well, the external interrupt line will remain asserted until 
> the device (via the PIC) stops interrupting the processor :-) 

Yes, the device keeps on interrupting the interrupt until the device clear its interrupted condition.

> Either because we mask at the PIC level (or raise the 
> priority) or because the condition goes away.
>  
> > I think regardless if you clear the device interrupt status, the 
> > system still  receive a pending interrupt once EE or GS = 1.
> 
> Why ? Depends on your PIC actually but if it's a sane one, it 
> won't "remember" a level interrupt that is gone. An edge 
> interrupt is a different matter and is latched.

But the interrupt controller like MPIC should leave this irq as pending if we don't ACK/EOI this irq at all if we just clear the device, right?

> 
> Some MPIC implementations tend to generate a spurrious IRQ in 
> the case of level IRQs going away. IE. they still remember an 
> event occurred and interrupt the processor, but on IACK 
> return the spurious vector. However that isn't guaranteed to 

Yes, this is just what I mean. No matter if this vector is still valid, the external interrupt exception always occur once EE = 1 again.

> be the case and it is perfectly ok (and a good
> idea) for the PIC to just stop asserting the external 
> interrupt line if the original device interrupt goes away 
> (and it's configured as level sensitive).

I don't remember MPIC can work with this way. So I'd like to look the manual again :)

Tiejun

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Benjamin Herrenschmidt]

On Thu, 2013-05-09 at 16:27 -0500, Scott Wood wrote:
> On 05/09/2013 07:37:42 AM, Benjamin Herrenschmidt wrote:
> > On Thu, 2013-05-09 at 17:44 +0800, tiejun.chen wrote:
> > >
> > > Actually in the case GS=1 even if EE=0, EXT/DEC/DBELL still occur  
> > as I
> > > recall.
> > 
> > Only if directed to the hypervisor.
> 
> This is always the case with KVM, right?  At least on booke...

Hrm, on A2 we could choose iirc. Well not DEC but EXT at least, I don't
remember about DBELL.

> > > > Case 1)
> > > >   -> Local_irq_disable()  will set soft_enabled = 0
> > > >   -> Now Externel interrupt happens, there we set PACA_IRQ_EE in
> > > irq_happened, Also clears EE in SRR1 and rfi. So interrupts are hard
> > > disabled. No more other interrupt gated by MSR.EE can happen. Looks
> > > like the idea here is to not let a device keep on inserting  
> > interrupt
> > > till the interrupt condition on device is cleared, right?
> > 
> > The external interrupt line is level sensitive normally, so we have to
> > mask MSR:EE in that case or the interrupt would keep re-occurring  
> > (note
> > that FSL has this concept of auto-acked interrupts via the on die MPIC
> > for which you can potentially use PACA_IRQ_EE_EDGE instead and avoid
> > having to mask MSR:EE).
> 
> Note that if we do this, we can no longer leave the interrupt vector in  
> EPR, and would have to track (potentially multiple different) pending  
> external interrupts in software.

Right, you can have a little queue in the PACA and leave EE disabled if
it's full. You can probably get away with a queue of 1 though :-)

Cheers,
Ben.

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 2403 bytes --]

On Fri, May 10, 2013 at 08:07:00AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2013-05-09 at 16:27 -0500, Scott Wood wrote:
> > On 05/09/2013 07:37:42 AM, Benjamin Herrenschmidt wrote:
> > > On Thu, 2013-05-09 at 17:44 +0800, tiejun.chen wrote:
> > > >
> > > > Actually in the case GS=1 even if EE=0, EXT/DEC/DBELL still occur  
> > > as I
> > > > recall.
> > > 
> > > Only if directed to the hypervisor.
> > 
> > This is always the case with KVM, right?  At least on booke...
> 
> Hrm, on A2 we could choose iirc. Well not DEC but EXT at least, I don't
> remember about DBELL.
> 
> > > > > Case 1)
> > > > >   -> Local_irq_disable()  will set soft_enabled = 0
> > > > >   -> Now Externel interrupt happens, there we set PACA_IRQ_EE in
> > > > irq_happened, Also clears EE in SRR1 and rfi. So interrupts are hard
> > > > disabled. No more other interrupt gated by MSR.EE can happen. Looks
> > > > like the idea here is to not let a device keep on inserting  
> > > interrupt
> > > > till the interrupt condition on device is cleared, right?
> > > 
> > > The external interrupt line is level sensitive normally, so we have to
> > > mask MSR:EE in that case or the interrupt would keep re-occurring  
> > > (note
> > > that FSL has this concept of auto-acked interrupts via the on die MPIC
> > > for which you can potentially use PACA_IRQ_EE_EDGE instead and avoid
> > > having to mask MSR:EE).
> > 
> > Note that if we do this, we can no longer leave the interrupt vector in  
> > EPR, and would have to track (potentially multiple different) pending  
> > external interrupts in software.
> 
> Right, you can have a little queue in the PACA and leave EE disabled if
> it's full. You can probably get away with a queue of 1 though :-)

So I would assume you will not pick up these two patches, right?
http://patchwork.ozlabs.org/patch/235530/
http://patchwork.ozlabs.org/patch/235532/

Anyway it is more easier to enable the external proxy by using this method.
But if you insist, I can respin a patch to use the method you suggested
since it will definitely reduce the window where the irq is hard disabled.

Thanks,
Kevin

> 
> Cheers,
> Ben.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Type: application/pgp-signature, Size: 490 bytes --]

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Benjamin Herrenschmidt]

On Fri, 2013-05-10 at 22:12 +0800, Kevin Hao wrote:
> So I would assume you will not pick up these two patches, right?
> http://patchwork.ozlabs.org/patch/235530/
> http://patchwork.ozlabs.org/patch/235532/
> 
> Anyway it is more easier to enable the external proxy by using this method.
> But if you insist, I can respin a patch to use the method you suggested
> since it will definitely reduce the window where the irq is hard disabled.

I would keep the EE_EDGE bit definition. I have no objection to a gradual
approach however for the other one where we apply it as is now to enable
coreint while you do a rework to make it better :-)

Cheers,
Ben.

Re: [RFC][KVM][PATCH 1/1] kvm:ppc:booke-64: soft-disable interrupts

[Benjamin Herrenschmidt]

On Sat, 2013-05-11 at 07:49 +1000, Benjamin Herrenschmidt wrote:
> I would keep the EE_EDGE bit definition. I have no objection to a gradual
> approach however for the other one where we apply it as is now to enable
> coreint while you do a rework to make it better :-)

Note also that I generally don't apply FSL related patches directly, I
rely on Kumar Gala picking them up so he's the one ultimately making
that choice.

Cheers,
Ben.