Patchwork KVM: PPC: BOOK3S: PR: Fix PURR and SPURR emulation

login
register
mail settings
Submitter Aneesh Kumar K.V
Date June 3, 2014, 12:16 p.m.
Message ID <1401797771-25606-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/355513/
State New
Headers show

Comments

Aneesh Kumar K.V - June 3, 2014, 12:16 p.m.
We use time base for PURR and SPURR emulation with PR KVM since we
are emulating a single threaded core. When using time base
we need to make sure that we don't accumulate time spent in the host
in PURR and SPURR value.

Also we don't need to emulate mtspr because both the registers are
hypervisor resource.

Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/kvm_book3s.h |  2 --
 arch/powerpc/include/asm/kvm_host.h   |  4 ++--
 arch/powerpc/kvm/book3s_emulate.c     | 16 ++++++++--------
 arch/powerpc/kvm/book3s_pr.c          | 10 ++++++++++
 4 files changed, 20 insertions(+), 12 deletions(-)
Paul Mackerras - June 4, 2014, 4:15 a.m.
On Tue, Jun 03, 2014 at 05:46:11PM +0530, Aneesh Kumar K.V wrote:
> We use time base for PURR and SPURR emulation with PR KVM since we
> are emulating a single threaded core. When using time base
> we need to make sure that we don't accumulate time spent in the host
> in PURR and SPURR value.

Mostly looks good except for this...

> @@ -170,6 +175,11 @@ void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu,
>  
>  out:
>  	preempt_enable();
> +	/*
> +	 * Update purr and spurr using time base
> +	 */
> +	vcpu->arch.purr += get_tb() - vcpu->arch.entry_tb;
> +	vcpu->arch.spurr += get_tb() - vcpu->arch.entry_tb;

You need to do those updates before the "out:" label.  Otherwise if
this function gets called with !svcpu->in_use (which can happen if
CONFIG_PREEMPT is enabled) we would do these updates a second time for
one guest exit.  The thing is that kvmppc_copy_from_svcpu() can get
called from kvmppc_core_vcpu_put_pr() if the vcpu task gets preempted
on the way out from the guest before we get to the regular call of
kvmppc_copy_from_svcpu().  It would then get called again when the
task gets to run, but this time it does nothing because svcpu->in_use
is false.

Paul.
--
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
Aneesh Kumar K.V - June 4, 2014, 10:45 a.m.
Paul Mackerras <paulus@samba.org> writes:

> On Tue, Jun 03, 2014 at 05:46:11PM +0530, Aneesh Kumar K.V wrote:
>> We use time base for PURR and SPURR emulation with PR KVM since we
>> are emulating a single threaded core. When using time base
>> we need to make sure that we don't accumulate time spent in the host
>> in PURR and SPURR value.
>
> Mostly looks good except for this...
>
>> @@ -170,6 +175,11 @@ void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu,
>>  
>>  out:
>>  	preempt_enable();
>> +	/*
>> +	 * Update purr and spurr using time base
>> +	 */
>> +	vcpu->arch.purr += get_tb() - vcpu->arch.entry_tb;
>> +	vcpu->arch.spurr += get_tb() - vcpu->arch.entry_tb;
>
> You need to do those updates before the "out:" label.  Otherwise if
> this function gets called with !svcpu->in_use (which can happen if
> CONFIG_PREEMPT is enabled) we would do these updates a second time for
> one guest exit.  The thing is that kvmppc_copy_from_svcpu() can get
> called from kvmppc_core_vcpu_put_pr() if the vcpu task gets preempted
> on the way out from the guest before we get to the regular call of
> kvmppc_copy_from_svcpu().  It would then get called again when the
> task gets to run, but this time it does nothing because svcpu->in_use
> is false.

Looking at the code, since we enable MSR.EE early now, we might possibly
end up calling this function late in the guest exit path. That
implies, we may account that time (time spent after a preempt immediately
following a guest exit) in purr/spurr. I guess that amount of inaccuracy is
ok, because that is the best we could do here ?

-aneesh

--
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
Aneesh Kumar K.V - June 4, 2014, 1:36 p.m.
"Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> writes:

> Paul Mackerras <paulus@samba.org> writes:
>
>> On Tue, Jun 03, 2014 at 05:46:11PM +0530, Aneesh Kumar K.V wrote:
>>> We use time base for PURR and SPURR emulation with PR KVM since we
>>> are emulating a single threaded core. When using time base
>>> we need to make sure that we don't accumulate time spent in the host
>>> in PURR and SPURR value.
>>
>> Mostly looks good except for this...
>>
>>> @@ -170,6 +175,11 @@ void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu,
>>>  
>>>  out:
>>>  	preempt_enable();
>>> +	/*
>>> +	 * Update purr and spurr using time base
>>> +	 */
>>> +	vcpu->arch.purr += get_tb() - vcpu->arch.entry_tb;
>>> +	vcpu->arch.spurr += get_tb() - vcpu->arch.entry_tb;
>>
>> You need to do those updates before the "out:" label.  Otherwise if
>> this function gets called with !svcpu->in_use (which can happen if
>> CONFIG_PREEMPT is enabled) we would do these updates a second time for
>> one guest exit.  The thing is that kvmppc_copy_from_svcpu() can get
>> called from kvmppc_core_vcpu_put_pr() if the vcpu task gets preempted
>> on the way out from the guest before we get to the regular call of
>> kvmppc_copy_from_svcpu().  It would then get called again when the
>> task gets to run, but this time it does nothing because svcpu->in_use
>> is false.
>
> Looking at the code, since we enable MSR.EE early now, we might possibly
> end up calling this function late in the guest exit path. That
> implies, we may account that time (time spent after a preempt immediately
> following a guest exit) in purr/spurr. I guess that amount of inaccuracy is
> ok, because that is the best we could do here ?
>

May be not, we do call kvmppc_copy_to_svcpu from preempt notifier
callbacks. That should ensure that we get the right value.

I have sent patch -V2 taking care of the above review comment.

-aneesh

--
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/include/asm/kvm_book3s.h b/arch/powerpc/include/asm/kvm_book3s.h
index f52f65694527..a20cc0bbd048 100644
--- a/arch/powerpc/include/asm/kvm_book3s.h
+++ b/arch/powerpc/include/asm/kvm_book3s.h
@@ -83,8 +83,6 @@  struct kvmppc_vcpu_book3s {
 	u64 sdr1;
 	u64 hior;
 	u64 msr_mask;
-	u64 purr_offset;
-	u64 spurr_offset;
 #ifdef CONFIG_PPC_BOOK3S_32
 	u32 vsid_pool[VSID_POOL_SIZE];
 	u32 vsid_next;
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index bb66d8b8efdf..4a58731a0a72 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -503,8 +503,8 @@  struct kvm_vcpu_arch {
 #ifdef CONFIG_BOOKE
 	u32 decar;
 #endif
-	u32 tbl;
-	u32 tbu;
+	/* Time base value when we entered the guest */
+	u64 entry_tb;
 	u32 tcr;
 	ulong tsr; /* we need to perform set/clr_bits() which requires ulong */
 	u32 ivor[64];
diff --git a/arch/powerpc/kvm/book3s_emulate.c b/arch/powerpc/kvm/book3s_emulate.c
index 3f295269af37..3565e775b61b 100644
--- a/arch/powerpc/kvm/book3s_emulate.c
+++ b/arch/powerpc/kvm/book3s_emulate.c
@@ -439,12 +439,6 @@  int kvmppc_core_emulate_mtspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong spr_val)
 		    (mfmsr() & MSR_HV))
 			vcpu->arch.hflags |= BOOK3S_HFLAG_DCBZ32;
 		break;
-	case SPRN_PURR:
-		to_book3s(vcpu)->purr_offset = spr_val - get_tb();
-		break;
-	case SPRN_SPURR:
-		to_book3s(vcpu)->spurr_offset = spr_val - get_tb();
-		break;
 	case SPRN_GQR0:
 	case SPRN_GQR1:
 	case SPRN_GQR2:
@@ -572,10 +566,16 @@  int kvmppc_core_emulate_mfspr_pr(struct kvm_vcpu *vcpu, int sprn, ulong *spr_val
 		*spr_val = 0;
 		break;
 	case SPRN_PURR:
-		*spr_val = get_tb() + to_book3s(vcpu)->purr_offset;
+		/*
+		 * On exit we would have updated purr
+		 */
+		*spr_val = vcpu->arch.purr;
 		break;
 	case SPRN_SPURR:
-		*spr_val = get_tb() + to_book3s(vcpu)->purr_offset;
+		/*
+		 * On exit we would have updated spurr
+		 */
+		*spr_val = vcpu->arch.spurr;
 		break;
 	case SPRN_GQR0:
 	case SPRN_GQR1:
diff --git a/arch/powerpc/kvm/book3s_pr.c b/arch/powerpc/kvm/book3s_pr.c
index 23367a7e44c3..c71ce784a72f 100644
--- a/arch/powerpc/kvm/book3s_pr.c
+++ b/arch/powerpc/kvm/book3s_pr.c
@@ -121,6 +121,11 @@  void kvmppc_copy_to_svcpu(struct kvmppc_book3s_shadow_vcpu *svcpu,
 	svcpu->shadow_fscr = vcpu->arch.shadow_fscr;
 #endif
 	svcpu->in_use = true;
+	/*
+	 * Now also save the current time base value. We use this
+	 * to find the guest purr and spurr value.
+	 */
+	vcpu->arch.entry_tb = get_tb();
 }
 
 /* Copy data touched by real-mode code from shadow vcpu back to vcpu */
@@ -170,6 +175,11 @@  void kvmppc_copy_from_svcpu(struct kvm_vcpu *vcpu,
 
 out:
 	preempt_enable();
+	/*
+	 * Update purr and spurr using time base
+	 */
+	vcpu->arch.purr += get_tb() - vcpu->arch.entry_tb;
+	vcpu->arch.spurr += get_tb() - vcpu->arch.entry_tb;
 }
 
 static int kvmppc_core_check_requests_pr(struct kvm_vcpu *vcpu)