Patchwork [36/37] KVM: PPC: booke: expose guest registers on irq reinject

login
register
mail settings
Submitter Alexander Graf
Date Feb. 24, 2012, 2:26 p.m.
Message ID <1330093591-19523-37-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/142856/
State New
Headers show

Comments

Alexander Graf - Feb. 24, 2012, 2:26 p.m.
When reinjecting an interrupt into the host interrupt handler after we're
back in host kernel land, let's tell the kernel about all the guest state
that the interrupt happened at.

This helps getting reasonable numbers out of perf.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 arch/powerpc/kvm/booke.c |   54 +++++++++++++++++++++++++++++++++------------
 1 files changed, 39 insertions(+), 15 deletions(-)
Scott Wood - Feb. 24, 2012, 11:40 p.m.
On 02/24/2012 08:26 AM, Alexander Graf wrote:
> +static void kvmppc_fill_pt_regs(struct kvm_vcpu *vcpu, struct pt_regs *regs)
>  {
> -	int r = RESUME_HOST;
> +	int i;
>  
> -	/* update before a new last_exit_type is rewritten */
> -	kvmppc_update_timing_stats(vcpu);
> +	for (i = 0; i < 32; i++)
> +		regs->gpr[i] = kvmppc_get_gpr(vcpu, i);
> +	regs->nip = vcpu->arch.pc;
> +	regs->msr = vcpu->arch.shared->msr;
> +	regs->ctr = vcpu->arch.ctr;
> +	regs->link = vcpu->arch.lr;
> +	regs->xer = kvmppc_get_xer(vcpu);
> +	regs->ccr = kvmppc_get_cr(vcpu);
> +	regs->dar = get_guest_dear(vcpu);
> +	regs->dsisr = get_guest_esr(vcpu);
> +}

How much overhead does this add to every interrupt?  Can't we keep this
to the minimum that perf cares about?

> +
> +static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu,
> +				     unsigned int exit_nr)
> +{
> +	struct pt_regs regs = *current->thread.regs;
>  
> +	kvmppc_fill_pt_regs(vcpu, &regs);

Why are you copying out of current->thread.regs?  That's old junk data,
set by some previous exception and possibly overwritten since.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Feb. 26, 2012, 11:59 a.m.
On 25.02.2012, at 00:40, Scott Wood wrote:

> On 02/24/2012 08:26 AM, Alexander Graf wrote:
>> +static void kvmppc_fill_pt_regs(struct kvm_vcpu *vcpu, struct pt_regs *regs)
>> {
>> -	int r = RESUME_HOST;
>> +	int i;
>> 
>> -	/* update before a new last_exit_type is rewritten */
>> -	kvmppc_update_timing_stats(vcpu);
>> +	for (i = 0; i < 32; i++)
>> +		regs->gpr[i] = kvmppc_get_gpr(vcpu, i);
>> +	regs->nip = vcpu->arch.pc;
>> +	regs->msr = vcpu->arch.shared->msr;
>> +	regs->ctr = vcpu->arch.ctr;
>> +	regs->link = vcpu->arch.lr;
>> +	regs->xer = kvmppc_get_xer(vcpu);
>> +	regs->ccr = kvmppc_get_cr(vcpu);
>> +	regs->dar = get_guest_dear(vcpu);
>> +	regs->dsisr = get_guest_esr(vcpu);
>> +}
> 
> How much overhead does this add to every interrupt?  Can't we keep this
> to the minimum that perf cares about?

I would rather not make assumptions on what perf cares about - maybe we want to one day implement "perf kvm" and then perf could rely on pretty much anything in there.

> 
>> +
>> +static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu,
>> +				     unsigned int exit_nr)
>> +{
>> +	struct pt_regs regs = *current->thread.regs;
>> 
>> +	kvmppc_fill_pt_regs(vcpu, &regs);
> 
> Why are you copying out of current->thread.regs?  That's old junk data,
> set by some previous exception and possibly overwritten since.

Because it gives us good default values for anything we don't set. Do you have other recommendations?


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Scott Wood - Feb. 27, 2012, 7:45 p.m.
On 02/26/2012 05:59 AM, Alexander Graf wrote:
> 
> On 25.02.2012, at 00:40, Scott Wood wrote:
> 
>> On 02/24/2012 08:26 AM, Alexander Graf wrote:
>>> +static void kvmppc_fill_pt_regs(struct kvm_vcpu *vcpu, struct pt_regs *regs)
>>> {
>>> -	int r = RESUME_HOST;
>>> +	int i;
>>>
>>> -	/* update before a new last_exit_type is rewritten */
>>> -	kvmppc_update_timing_stats(vcpu);
>>> +	for (i = 0; i < 32; i++)
>>> +		regs->gpr[i] = kvmppc_get_gpr(vcpu, i);
>>> +	regs->nip = vcpu->arch.pc;
>>> +	regs->msr = vcpu->arch.shared->msr;
>>> +	regs->ctr = vcpu->arch.ctr;
>>> +	regs->link = vcpu->arch.lr;
>>> +	regs->xer = kvmppc_get_xer(vcpu);
>>> +	regs->ccr = kvmppc_get_cr(vcpu);
>>> +	regs->dar = get_guest_dear(vcpu);
>>> +	regs->dsisr = get_guest_esr(vcpu);
>>> +}
>>
>> How much overhead does this add to every interrupt?  Can't we keep this
>> to the minimum that perf cares about?
> 
> I would rather not make assumptions on what perf cares about - maybe we want to one day implement "perf kvm" and then perf could rely on pretty much anything in there.

In that case I think we should be populating a real pt_regs from the
start, as in my original patchset.

I only agreed to take it out because I thought the set of things we'd
copy would be minimal.  This seems like a lot of overhead.

I'm not familiar with "perf kvm", but if it's kvm-specific surely the
KVM code should know/dictate what it can rely on?  Or maybe there can be
a debug option that enables full pt_regs (similar to exit timing)?

Could we just set regs to NULL when the debug option isn't enabled?

>>> +static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu,
>>> +				     unsigned int exit_nr)
>>> +{
>>> +	struct pt_regs regs = *current->thread.regs;
>>>
>>> +	kvmppc_fill_pt_regs(vcpu, &regs);
>>
>> Why are you copying out of current->thread.regs?  That's old junk data,
>> set by some previous exception and possibly overwritten since.
> 
> Because it gives us good default values for anything we don't set. Do you have other recommendations?

It does not give good default values for anything.  It is junk,
unallocated memory, overwritten by who knows what.  Same as the memory
you're copying to.

To avoid garbage in fields we don't set, fill it with zeroes first.

-Scott

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

Patch

diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
index 423701b..709fd45 100644
--- a/arch/powerpc/kvm/booke.c
+++ b/arch/powerpc/kvm/booke.c
@@ -593,37 +593,61 @@  static int emulation_exit(struct kvm_run *run, struct kvm_vcpu *vcpu)
 	}
 }
 
-/**
- * kvmppc_handle_exit
- *
- * Return value is in the form (errcode<<2 | RESUME_FLAG_HOST | RESUME_FLAG_NV)
- */
-int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
-                       unsigned int exit_nr)
+static void kvmppc_fill_pt_regs(struct kvm_vcpu *vcpu, struct pt_regs *regs)
 {
-	int r = RESUME_HOST;
+	int i;
 
-	/* update before a new last_exit_type is rewritten */
-	kvmppc_update_timing_stats(vcpu);
+	for (i = 0; i < 32; i++)
+		regs->gpr[i] = kvmppc_get_gpr(vcpu, i);
+	regs->nip = vcpu->arch.pc;
+	regs->msr = vcpu->arch.shared->msr;
+	regs->ctr = vcpu->arch.ctr;
+	regs->link = vcpu->arch.lr;
+	regs->xer = kvmppc_get_xer(vcpu);
+	regs->ccr = kvmppc_get_cr(vcpu);
+	regs->dar = get_guest_dear(vcpu);
+	regs->dsisr = get_guest_esr(vcpu);
+}
+
+static void kvmppc_restart_interrupt(struct kvm_vcpu *vcpu,
+				     unsigned int exit_nr)
+{
+	struct pt_regs regs = *current->thread.regs;
 
+	kvmppc_fill_pt_regs(vcpu, &regs);
 	switch (exit_nr) {
 	case BOOKE_INTERRUPT_EXTERNAL:
-		do_IRQ(current->thread.regs);
+		do_IRQ(&regs);
 		break;
-
 	case BOOKE_INTERRUPT_DECREMENTER:
-		timer_interrupt(current->thread.regs);
+		timer_interrupt(&regs);
 		break;
-
 #if defined(CONFIG_PPC_FSL_BOOK3E) || defined(CONFIG_PPC_BOOK3E_64)
 	case BOOKE_INTERRUPT_DOORBELL:
-		doorbell_exception(current->thread.regs);
+		doorbell_exception(&regs);
 		break;
 #endif
 	case BOOKE_INTERRUPT_MACHINE_CHECK:
 		/* FIXME */
 		break;
 	}
+}
+
+/**
+ * kvmppc_handle_exit
+ *
+ * Return value is in the form (errcode<<2 | RESUME_FLAG_HOST | RESUME_FLAG_NV)
+ */
+int kvmppc_handle_exit(struct kvm_run *run, struct kvm_vcpu *vcpu,
+                       unsigned int exit_nr)
+{
+	int r = RESUME_HOST;
+
+	/* 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();