Re: [RFC v2 02/10] KVM: arm/arm64: Move cntvoff to each timer context

[Marc Zyngier]

On 30/01/17 14:45, Christoffer Dall wrote:
> On Sun, Jan 29, 2017 at 11:54:05AM +0000, Marc Zyngier wrote:
>> On Fri, Jan 27 2017 at 01:04:52 AM, Jintack Lim <jintack@xxxxxxxxxxxxxxx> wrote:
>>> Make cntvoff per each timer context. This is helpful to abstract kvm
>>> timer functions to work with timer context without considering timer
>>> types (e.g. physical timer or virtual timer).
>>>
>>> This also would pave the way for ever doing adjustments of the cntvoff
>>> on a per-CPU basis if that should ever make sense.
>>>
>>> Signed-off-by: Jintack Lim <jintack@xxxxxxxxxxxxxxx>
>>> ---
>>>  arch/arm/include/asm/kvm_host.h   |  6 +++---
>>>  arch/arm64/include/asm/kvm_host.h |  4 ++--
>>>  include/kvm/arm_arch_timer.h      |  8 +++-----
>>>  virt/kvm/arm/arch_timer.c         | 26 ++++++++++++++++++++------
>>>  virt/kvm/arm/hyp/timer-sr.c       |  3 +--
>>>  5 files changed, 29 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>>> index d5423ab..f5456a9 100644
>>> --- a/arch/arm/include/asm/kvm_host.h
>>> +++ b/arch/arm/include/asm/kvm_host.h
>>> @@ -60,9 +60,6 @@ struct kvm_arch {
>>>  	/* The last vcpu id that ran on each physical CPU */
>>>  	int __percpu *last_vcpu_ran;
>>>  
>>> -	/* Timer */
>>> -	struct arch_timer_kvm	timer;
>>> -
>>>  	/*
>>>  	 * Anything that is not used directly from assembly code goes
>>>  	 * here.
>>> @@ -75,6 +72,9 @@ struct kvm_arch {
>>>  	/* Stage-2 page table */
>>>  	pgd_t *pgd;
>>>  
>>> +	/* A lock to synchronize cntvoff among all vtimer context of vcpus */
>>> +	spinlock_t cntvoff_lock;
>>
>> Is there any condition where we need this to be a spinlock? I would have
>> thought that a mutex should have been enough, as this should only be
>> updated on migration or initialization. Not that it matters much in this
>> case, but I wondered if there is something I'm missing.
>>
> 
> I would think the critical section is small enough that a spinlock makes
> sense, but what I don't think we need is to add the additional lock.
> 
> I think just taking the kvm->lock should be sufficient, which happens to
> be a mutex, and while that may be a bit slower to take than the
> spinlock, it's not in the critical path so let's just keep things
> simple.
> 
> Perhaps this what Marc also meant.

That would be the logical conclusion, assuming that we can sleep on this
path.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...