Re: [PATCH] KVM: PPC: BOOK3S: PR: Fix PURR and SPURR emulation

["Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>]

"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