[PATCH 0/3] Improve Decrementor Implementation

[PATCH 0/3] Improve Decrementor Implementation

[Alexander Graf]

We currently have an ugly hack called AGGRESSIVE_DEC that makes the Decrementor
either work well for PPC32 or PPC64 targets.

This patchset removes that hack, bringing the decrementor implementation closer
to real hardware.

Alexander Graf (3):
  Move vector to irqprio resolving to separate function
  Improve DEC handling
  Remove AGGRESSIVE_DEC

 arch/powerpc/include/asm/kvm_ppc.h |    1 +
 arch/powerpc/kvm/book3s.c          |   45 ++++++++++++++++++++---------------
 arch/powerpc/kvm/booke.c           |    5 ++++
 arch/powerpc/kvm/emulate.c         |    1 +
 4 files changed, 33 insertions(+), 19 deletions(-)

[PATCH 1/3] Move vector to irqprio resolving to separate function

[Alexander Graf]

We're using a switch table to find the irqprio that belongs to a specific
interrupt vector. This table is part of the interrupt inject logic.

Since we'll add a new function to stop interrupts, let's move this table
out of the injection logic into a separate function.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/book3s.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 3e294bd..241795b 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -125,11 +125,10 @@ void kvmppc_inject_interrupt(struct kvm_vcpu *vcpu, int vec, u64 flags)
 	vcpu->arch.mmu.reset_msr(vcpu);
 }
 
-void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
+static int kvmppc_book3s_vec2irqprio(unsigned int vec)
 {
 	unsigned int prio;
 
-	vcpu->stat.queue_intr++;
 	switch (vec) {
 	case 0x100: prio = BOOK3S_IRQPRIO_SYSTEM_RESET;		break;
 	case 0x200: prio = BOOK3S_IRQPRIO_MACHINE_CHECK;	break;
@@ -149,7 +148,15 @@ void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
 	default:    prio = BOOK3S_IRQPRIO_MAX;			break;
 	}
 
-	set_bit(prio, &vcpu->arch.pending_exceptions);
+	return prio;
+}
+
+void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
+{
+	vcpu->stat.queue_intr++;
+
+	set_bit(kvmppc_book3s_vec2irqprio(vec),
+		&vcpu->arch.pending_exceptions);
 #ifdef EXIT_DEBUG
 	printk(KERN_INFO "Queueing interrupt %x\n", vec);
 #endif
-- 
1.6.0.2

[PATCH 2/3] Improve DEC handling

[Alexander Graf]

We treated the DEC interrupt like an edge based one. This is not true for
Book3s. The DEC keeps firing until mtdec is issued again and thus clears
the interrupt line.

So let's implement this logic in KVM too. This patch moves the line clearing
from the firing of the interrupt to the mtdec emulation.

This makes PPC64 guests work without AGGRESSIVE_DEC defined.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/include/asm/kvm_ppc.h |    1 +
 arch/powerpc/kvm/book3s.c          |   16 +++++++++++++++-
 arch/powerpc/kvm/booke.c           |    5 +++++
 arch/powerpc/kvm/emulate.c         |    1 +
 4 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 269ee46..144d3aa 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -80,6 +80,7 @@ extern void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu);
 
 extern void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu);
 extern int kvmppc_core_pending_dec(struct kvm_vcpu *vcpu);
+extern void kvmppc_core_stop_dec(struct kvm_vcpu *vcpu);
 extern void kvmppc_core_queue_program(struct kvm_vcpu *vcpu);
 extern void kvmppc_core_queue_dec(struct kvm_vcpu *vcpu);
 extern void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 241795b..1b3482b 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -151,6 +151,13 @@ static int kvmppc_book3s_vec2irqprio(unsigned int vec)
 	return prio;
 }
 
+static void kvmppc_book3s_stop_interrupt(struct kvm_vcpu *vcpu,
+					 unsigned int vec)
+{
+	clear_bit(kvmppc_book3s_vec2irqprio(vec),
+		  &vcpu->arch.pending_exceptions);
+}
+
 void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
 {
 	vcpu->stat.queue_intr++;
@@ -178,6 +185,11 @@ int kvmppc_core_pending_dec(struct kvm_vcpu *vcpu)
 	return test_bit(BOOK3S_INTERRUPT_DECREMENTER >> 7, &vcpu->arch.pending_exceptions);
 }
 
+void kvmppc_core_stop_dec(struct kvm_vcpu *vcpu)
+{
+	kvmppc_book3s_stop_interrupt(vcpu, BOOK3S_INTERRUPT_DECREMENTER);
+}
+
 void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
                                 struct kvm_interrupt *irq)
 {
@@ -275,7 +287,9 @@ void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu)
 #endif
 	priority = __ffs(*pending);
 	while (priority <= (sizeof(unsigned int) * 8)) {
-		if (kvmppc_book3s_irqprio_deliver(vcpu, priority)) {
+		if (kvmppc_book3s_irqprio_deliver(vcpu, priority) &&
+		    (priority != BOOK3S_IRQPRIO_DECREMENTER)) {
+			/* DEC interrupts get cleared by mtdec */
 			clear_bit(priority, &vcpu->arch.pending_exceptions);
 			break;
 		}
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 06f5a9e..79dcecb 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -97,6 +97,11 @@ int kvmppc_core_pending_dec(struct kvm_vcpu *vcpu)
 	return test_bit(BOOKE_IRQPRIO_DECREMENTER, &vcpu->arch.pending_exceptions);
 }
 
+void kvmppc_core_stop_dec(struct kvm_vcpu *vcpu)
+{
+	clear_bit(BOOKE_IRQPRIO_DECREMENTER, &vcpu->arch.pending_exceptions);
+}
+
 void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
                                 struct kvm_interrupt *irq)
 {
diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index 4a9ac66..5e6511c 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -82,6 +82,7 @@ void kvmppc_emulate_dec(struct kvm_vcpu *vcpu)
 	unsigned long dec_nsec;
 
 	pr_debug("mtDEC: %x\n", vcpu->arch.dec);
+	kvmppc_core_stop_dec(vcpu);
 #ifdef CONFIG_PPC64
 	/* POWER4+ triggers a dec interrupt if the value is < 0 */
 	if (vcpu->arch.dec & 0x80000000) {
-- 
1.6.0.2

[PATCH 3/3] Remove AGGRESSIVE_DEC

[Alexander Graf]

Because we now emulate the DEC interrupt according to real life behavior,
there's no need to keep the AGGRESSIVE_DEC hack around.

Let's just remove it.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/book3s.c |   16 +---------------
 1 files changed, 1 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 1b3482b..354a0e8 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -34,12 +34,6 @@
 /* #define EXIT_DEBUG */
 /* #define EXIT_DEBUG_SIMPLE */
 
-/* Without AGGRESSIVE_DEC we only fire off a DEC interrupt when DEC turns 0.
- * When set, we retrigger a DEC interrupt after that if DEC <= 0.
- * PPC32 Linux runs faster without AGGRESSIVE_DEC, PPC64 Linux requires it. */
-
-/* #define AGGRESSIVE_DEC */
-
 struct kvm_stats_debugfs_item debugfs_entries[] = {
 	{ "exits",       VCPU_STAT(sum_exits) },
 	{ "mmio",        VCPU_STAT(mmio_exits) },
@@ -81,7 +75,7 @@ void kvmppc_core_vcpu_put(struct kvm_vcpu *vcpu)
 	to_book3s(vcpu)->slb_shadow_max = get_paca()->kvm_slb_max;
 }
 
-#if defined(AGGRESSIVE_DEC) || defined(EXIT_DEBUG)
+#if defined(EXIT_DEBUG)
 static u32 kvmppc_get_dec(struct kvm_vcpu *vcpu)
 {
 	u64 jd = mftb() - vcpu->arch.dec_jiffies;
@@ -273,14 +267,6 @@ void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu)
 	unsigned long *pending = &vcpu->arch.pending_exceptions;
 	unsigned int priority;
 
-	/* XXX be more clever here - no need to mftb() on every entry */
-	/* Issue DEC again if it's still active */
-#ifdef AGGRESSIVE_DEC
-	if (vcpu->arch.msr & MSR_EE)
-		if (kvmppc_get_dec(vcpu) & 0x80000000)
-			kvmppc_core_queue_dec(vcpu);
-#endif
-
 #ifdef EXIT_DEBUG
 	if (vcpu->arch.pending_exceptions)
 		printk(KERN_EMERG "KVM: Check pending: %lx\n", vcpu->arch.pending_exceptions);
-- 
1.6.0.2

Re: [PATCH 2/3] Improve DEC handling

[Hollis Blanchard]

On Mon, Dec 21, 2009 at 6:22 AM, Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org> wrote:
> We treated the DEC interrupt like an edge based one. This is not true for
> Book3s. The DEC keeps firing until mtdec is issued again and thus clears
> the interrupt line.

That's not quite right. The decrementer keeps firing until the top bit
is cleared, i.e. with mtdec. However, not *every* mtdec clears it.
(Also, I'm pretty sure this varies between Book 3S implementations,
e.g. 970 behaves differently than POWERn. I don't remember specific
values of <n> though, and I could be misremembering...)

So is this the failure mode?
- a decrementer interrupt is delivered
- guest does *not* issue mtdec to clear it (ppc64's lazy interrupt disabling?)
- guest expects a second decrementer interrupt, but KVM doesn't deliver one

In that case, it seems like the real fix would be something like this:

 void kvmppc_emulate_dec(struct kvm_vcpu *vcpu)
 {
 	unsigned long dec_nsec;

 	pr_debug("mtDEC: %x\n", vcpu->arch.dec);
 #ifdef CONFIG_PPC64
 	/* POWER4+ triggers a dec interrupt if the value is < 0 */
 	if (vcpu->arch.dec & 0x80000000) {
 		hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
 		kvmppc_core_queue_dec(vcpu);
+		/* keep queuing interrupts until guest clears high MSR bit */
+ 		hrtimer_start(&vcpu->arch.dec_timer, ktime_set(0, 100),
+ 			      HRTIMER_MODE_REL);
 		return;
 	}
 #endif

-Hollis

Re: [PATCH 2/3] Improve DEC handling

[Hollis Blanchard]

On Mon, Dec 21, 2009 at 10:13 AM, Hollis Blanchard
<hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org> wrote:
>  void kvmppc_emulate_dec(struct kvm_vcpu *vcpu)
>  {
>        unsigned long dec_nsec;
>
>        pr_debug("mtDEC: %x\n", vcpu->arch.dec);
>  #ifdef CONFIG_PPC64
>        /* POWER4+ triggers a dec interrupt if the value is < 0 */
>        if (vcpu->arch.dec & 0x80000000) {
>                hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
>                kvmppc_core_queue_dec(vcpu);
> +               /* keep queuing interrupts until guest clears high MSR bit */
> +               hrtimer_start(&vcpu->arch.dec_timer, ktime_set(0, 100),
> +                             HRTIMER_MODE_REL);
>                return;
>        }
>  #endif

Of course, removing the hardcoded 100-ns timer would be better, and
indeed we can do that. What we *really* want is to key off of MSR[EE]
changes (there's no point in queuing anything until then). So why not
move your "AGGRESSIVE_DEC" check into Book 3S's kvmppc_set_msr()?

-Hollis

Re: [PATCH 2/3] Improve DEC handling

[Alexander Graf]

Hollis Blanchard wrote:
> On Mon, Dec 21, 2009 at 10:13 AM, Hollis Blanchard
> <hollis-yUx37fBWTUITNcAmw9vGhQ@public.gmane.org> wrote:
>   
>>  void kvmppc_emulate_dec(struct kvm_vcpu *vcpu)
>>  {
>>        unsigned long dec_nsec;
>>
>>        pr_debug("mtDEC: %x\n", vcpu->arch.dec);
>>  #ifdef CONFIG_PPC64
>>        /* POWER4+ triggers a dec interrupt if the value is < 0 */
>>        if (vcpu->arch.dec & 0x80000000) {
>>                hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
>>                kvmppc_core_queue_dec(vcpu);
>> +               /* keep queuing interrupts until guest clears high MSR bit */
>> +               hrtimer_start(&vcpu->arch.dec_timer, ktime_set(0, 100),
>> +                             HRTIMER_MODE_REL);
>>                return;
>>        }
>>  #endif
>>     
>
> Of course, removing the hardcoded 100-ns timer would be better, and
> indeed we can do that. What we *really* want is to key off of MSR[EE]
> changes (there's no point in queuing anything until then).

Yes. mtmsr with EE=1 triggers a #VMEXIT which then goes off into a VM
entry which on entering checks that the DEC interrupt is still active
and fires it.

> So why not
> move your "AGGRESSIVE_DEC" check into Book 3S's kvmppc_set_msr()?
>   

The idea was to get rid of AGGRESSIVE_DEC :-).

Alex

Re: [PATCH 2/3] Improve DEC handling

[Hollis Blanchard]

For the record, we've discussed more by IRC, and I think revised
patches will be forthcoming.

-Hollis

Re: [PATCH 2/3] Improve DEC handling

[Alexander Graf]

Hollis Blanchard wrote:
> On Mon, Dec 21, 2009 at 6:22 AM, Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org> wrote:
>   
>> We treated the DEC interrupt like an edge based one. This is not true for
>> Book3s. The DEC keeps firing until mtdec is issued again and thus clears
>> the interrupt line.
>>     
>
> That's not quite right. The decrementer keeps firing until the top bit
> is cleared, i.e. with mtdec. However, not *every* mtdec clears it.
>   

Right, that's we we fire a dec interrupt off whenever we mtdec with the
top bit set.

> (Also, I'm pretty sure this varies between Book 3S implementations,
> e.g. 970 behaves differently than POWERn. I don't remember specific
> values of <n> though, and I could be misremembering...)
>   

IIRC only the embedded cores were different. But I could be wrong. How
do I find out?

> So is this the failure mode?
> - a decrementer interrupt is delivered
> - guest does *not* issue mtdec to clear it (ppc64's lazy interrupt disabling?)
> - guest expects a second decrementer interrupt, but KVM doesn't deliver one
>
> In that case, it seems like the real fix would be something like this:
>
>  void kvmppc_emulate_dec(struct kvm_vcpu *vcpu)
>  {
>  	unsigned long dec_nsec;
>
>  	pr_debug("mtDEC: %x\n", vcpu->arch.dec);
>  #ifdef CONFIG_PPC64
>  	/* POWER4+ triggers a dec interrupt if the value is < 0 */
>  	if (vcpu->arch.dec & 0x80000000) {
>  		hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
>  		kvmppc_core_queue_dec(vcpu);
> +		/* keep queuing interrupts until guest clears high MSR bit */
> + 		hrtimer_start(&vcpu->arch.dec_timer, ktime_set(0, 100),
> + 			      HRTIMER_MODE_REL);
>   

This code path is only triggered when the guest mtdecs with a negative
value.

But I understand what you're trying to suggest and I think it's a bad
idea. We don't want to poll the guest for interrupt enablement.

On a real CPU the DEC interrupt keeps being active when the DEC register
is negative. And that's exactly what this patch implements, no? That way
we are automatically event-based and everyone's happy.

Alex

[PATCH 0/3] Improve Decrementor Implementation v2

[Alexander Graf]

We currently have an ugly hack called AGGRESSIVE_DEC that makes the Decrementor
either work well for PPC32 or PPC64 targets.

This patchset removes that hack, bringing the decrementor implementation closer
to real hardware.

V1 -> V2:

  - make DEC clearing code on mtdec book3s specific
  - rename stop -> dequeue

Alexander Graf (3):
  Move vector to irqprio resolving to separate function
  Improve DEC handling
  Remove AGGRESSIVE_DEC

 arch/powerpc/include/asm/kvm_ppc.h |    1 +
 arch/powerpc/kvm/book3s.c          |   45 ++++++++++++++++++++---------------
 arch/powerpc/kvm/booke.c           |    5 ++++
 arch/powerpc/kvm/emulate.c         |    3 ++
 4 files changed, 35 insertions(+), 19 deletions(-)

[PATCH 2/3] Improve DEC handling

[Alexander Graf]

We treated the DEC interrupt like an edge based one. This is not true for
Book3s. The DEC keeps firing until mtdec is issued again and thus clears
the interrupt line.

So let's implement this logic in KVM too. This patch moves the line clearing
from the firing of the interrupt to the mtdec emulation.

This makes PPC64 guests work without AGGRESSIVE_DEC defined.

Signed-off-by: Alexander Graf <agraf-l3A5Bk7waGM@public.gmane.org>

---

V1 -> V2:

  - make DEC clearing code on mtdec book3s specific
  - rename stop -> dequeue
---
 arch/powerpc/include/asm/kvm_ppc.h |    1 +
 arch/powerpc/kvm/book3s.c          |   16 +++++++++++++++-
 arch/powerpc/kvm/booke.c           |    5 +++++
 arch/powerpc/kvm/emulate.c         |    3 +++
 4 files changed, 24 insertions(+), 1 deletions(-)

diff --git a/arch/powerpc/include/asm/kvm_ppc.h b/arch/powerpc/include/asm/kvm_ppc.h
index 269ee46..abfd0c4 100644
--- a/arch/powerpc/include/asm/kvm_ppc.h
+++ b/arch/powerpc/include/asm/kvm_ppc.h
@@ -82,6 +82,7 @@ extern void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu);
 extern int kvmppc_core_pending_dec(struct kvm_vcpu *vcpu);
 extern void kvmppc_core_queue_program(struct kvm_vcpu *vcpu);
 extern void kvmppc_core_queue_dec(struct kvm_vcpu *vcpu);
+extern void kvmppc_core_dequeue_dec(struct kvm_vcpu *vcpu);
 extern void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
                                        struct kvm_interrupt *irq);
 
diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
index 241795b..fd3ad6c 100644
--- a/arch/powerpc/kvm/book3s.c
+++ b/arch/powerpc/kvm/book3s.c
@@ -151,6 +151,13 @@ static int kvmppc_book3s_vec2irqprio(unsigned int vec)
 	return prio;
 }
 
+static void kvmppc_book3s_dequeue_irqprio(struct kvm_vcpu *vcpu,
+					  unsigned int vec)
+{
+	clear_bit(kvmppc_book3s_vec2irqprio(vec),
+		  &vcpu->arch.pending_exceptions);
+}
+
 void kvmppc_book3s_queue_irqprio(struct kvm_vcpu *vcpu, unsigned int vec)
 {
 	vcpu->stat.queue_intr++;
@@ -178,6 +185,11 @@ int kvmppc_core_pending_dec(struct kvm_vcpu *vcpu)
 	return test_bit(BOOK3S_INTERRUPT_DECREMENTER >> 7, &vcpu->arch.pending_exceptions);
 }
 
+void kvmppc_core_dequeue_dec(struct kvm_vcpu *vcpu)
+{
+	kvmppc_book3s_dequeue_irqprio(vcpu, BOOK3S_INTERRUPT_DECREMENTER);
+}
+
 void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
                                 struct kvm_interrupt *irq)
 {
@@ -275,7 +287,9 @@ void kvmppc_core_deliver_interrupts(struct kvm_vcpu *vcpu)
 #endif
 	priority = __ffs(*pending);
 	while (priority <= (sizeof(unsigned int) * 8)) {
-		if (kvmppc_book3s_irqprio_deliver(vcpu, priority)) {
+		if (kvmppc_book3s_irqprio_deliver(vcpu, priority) &&
+		    (priority != BOOK3S_IRQPRIO_DECREMENTER)) {
+			/* DEC interrupts get cleared by mtdec */
 			clear_bit(priority, &vcpu->arch.pending_exceptions);
 			break;
 		}
diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 06f5a9e..d8b6342 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -97,6 +97,11 @@ int kvmppc_core_pending_dec(struct kvm_vcpu *vcpu)
 	return test_bit(BOOKE_IRQPRIO_DECREMENTER, &vcpu->arch.pending_exceptions);
 }
 
+void kvmppc_core_dequeue_dec(struct kvm_vcpu *vcpu)
+{
+	clear_bit(BOOKE_IRQPRIO_DECREMENTER, &vcpu->arch.pending_exceptions);
+}
+
 void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
                                 struct kvm_interrupt *irq)
 {
diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index 4a9ac66..303457b 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -83,6 +83,9 @@ void kvmppc_emulate_dec(struct kvm_vcpu *vcpu)
 
 	pr_debug("mtDEC: %x\n", vcpu->arch.dec);
 #ifdef CONFIG_PPC64
+	/* mtdec lowers the interrupt line when positive. */
+	kvmppc_core_dequeue_dec(vcpu);
+
 	/* POWER4+ triggers a dec interrupt if the value is < 0 */
 	if (vcpu->arch.dec & 0x80000000) {
 		hrtimer_try_to_cancel(&vcpu->arch.dec_timer);
-- 
1.6.0.2