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

On Mon, Jan 30, 2017 at 9:51 AM, Marc Zyngier [off-list ref] wrote:

On 30/01/17 14:45, Christoffer Dall wrote:

quoted

On Sun, Jan 29, 2017 at 11:54:05AM +0000, Marc Zyngier wrote:

quoted

On Fri, Jan 27 2017 at 01:04:52 AM, Jintack Lim [off-list ref] wrote:

quoted

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 <redacted>
---
 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.

All right. I'll take kvm->lock there.

Thanks,
Jintack

Thanks,

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