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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, Jan 29, 2017 at 6:54 AM, Marc Zyngier <marc.zyngier@xxxxxxx> 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.
>
>> +
>>       /* Interrupt controller */
>>       struct vgic_dist        vgic;
>>       int max_vcpus;
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
>> index e505038..23749a8 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -71,8 +71,8 @@ struct kvm_arch {
>>       /* Interrupt controller */
>>       struct vgic_dist        vgic;
>>
>> -     /* Timer */
>> -     struct arch_timer_kvm   timer;
>> +     /* A lock to synchronize cntvoff among all vtimer context of vcpus */
>> +     spinlock_t cntvoff_lock;
>>  };
>>
>>  #define KVM_NR_MEM_OBJS     40
>> diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
>> index daad3c1..1b9c988 100644
>> --- a/include/kvm/arm_arch_timer.h
>> +++ b/include/kvm/arm_arch_timer.h
>> @@ -23,11 +23,6 @@
>>  #include <linux/hrtimer.h>
>>  #include <linux/workqueue.h>
>>
>> -struct arch_timer_kvm {
>> -     /* Virtual offset */
>> -     u64                     cntvoff;
>> -};
>> -
>>  struct arch_timer_context {
>>       /* Registers: control register, timer value */
>>       u32                             cnt_ctl;
>> @@ -38,6 +33,9 @@ struct arch_timer_context {
>>
>>       /* Active IRQ state caching */
>>       bool                            active_cleared_last;
>> +
>> +     /* Virtual offset */
>> +     u64                     cntvoff;
>>  };
>>
>>  struct arch_timer_cpu {
>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
>> index 6740efa..fa4c042 100644
>> --- a/virt/kvm/arm/arch_timer.c
>> +++ b/virt/kvm/arm/arch_timer.c
>> @@ -101,9 +101,10 @@ static void kvm_timer_inject_irq_work(struct work_struct *work)
>>  static u64 kvm_timer_compute_delta(struct kvm_vcpu *vcpu)
>>  {
>>       u64 cval, now;
>> +     struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>>
>> -     cval = vcpu_vtimer(vcpu)->cnt_cval;
>> -     now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
>> +     cval = vtimer->cnt_cval;
>> +     now = kvm_phys_timer_read() - vtimer->cntvoff;
>>
>>       if (now < cval) {
>>               u64 ns;
>> @@ -159,7 +160,7 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
>>               return false;
>>
>>       cval = vtimer->cnt_cval;
>> -     now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
>> +     now = kvm_phys_timer_read() - vtimer->cntvoff;
>>
>>       return cval <= now;
>>  }
>> @@ -353,10 +354,23 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>>       return 0;
>>  }
>>
>> +/* Make the updates of cntvoff for all vtimer contexts atomic */
>> +static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff)
>
> Arguably, this acts on the VM itself and not a single vcpu. maybe you
> should consider passing the struct kvm pointer to reflect this.
>

Yes, that would be better.

>> +{
>> +     int i;
>> +
>> +     spin_lock(&vcpu->kvm->arch.cntvoff_lock);
>> +     kvm_for_each_vcpu(i, vcpu, vcpu->kvm)
>> +             vcpu_vtimer(vcpu)->cntvoff = cntvoff;
>> +     spin_unlock(&vcpu->kvm->arch.cntvoff_lock);
>> +}
>> +
>>  void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
>>  {
>>       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>
>> +     update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
>
> Maybe a comment indicating that we recompute CNTVOFF for all vcpus would
> be welcome (this is not a change in semantics, but it was never obvious
> in the existing code).

I'll add a comment. In fact, I was told to make cntvoff synchronized
across all the vcpus, but I'm afraid that I understand why. Could you
explain me where this constraint comes from?

>
>> +
>>       INIT_WORK(&timer->expired, kvm_timer_inject_irq_work);
>>       hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>>       timer->timer.function = kvm_timer_expire;
>> @@ -376,7 +390,7 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, u64 value)
>>               vtimer->cnt_ctl = value;
>>               break;
>>       case KVM_REG_ARM_TIMER_CNT:
>> -             vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read() - value;
>> +             update_vtimer_cntvoff(vcpu, kvm_phys_timer_read() - value);
>>               break;
>>       case KVM_REG_ARM_TIMER_CVAL:
>>               vtimer->cnt_cval = value;
>> @@ -397,7 +411,7 @@ u64 kvm_arm_timer_get_reg(struct kvm_vcpu *vcpu, u64 regid)
>>       case KVM_REG_ARM_TIMER_CTL:
>>               return vtimer->cnt_ctl;
>>       case KVM_REG_ARM_TIMER_CNT:
>> -             return kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
>> +             return kvm_phys_timer_read() - vtimer->cntvoff;
>>       case KVM_REG_ARM_TIMER_CVAL:
>>               return vtimer->cnt_cval;
>>       }
>> @@ -511,7 +525,7 @@ int kvm_timer_enable(struct kvm_vcpu *vcpu)
>>
>>  void kvm_timer_init(struct kvm *kvm)
>>  {
>> -     kvm->arch.timer.cntvoff = kvm_phys_timer_read();
>> +     spin_lock_init(&kvm->arch.cntvoff_lock);
>>  }
>>
>>  /*
>> diff --git a/virt/kvm/arm/hyp/timer-sr.c b/virt/kvm/arm/hyp/timer-sr.c
>> index 0cf0895..4734915 100644
>> --- a/virt/kvm/arm/hyp/timer-sr.c
>> +++ b/virt/kvm/arm/hyp/timer-sr.c
>> @@ -53,7 +53,6 @@ void __hyp_text __timer_save_state(struct kvm_vcpu *vcpu)
>>
>>  void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
>>  {
>> -     struct kvm *kvm = kern_hyp_va(vcpu->kvm);
>>       struct arch_timer_cpu *timer = &vcpu->arch.timer_cpu;
>>       struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
>>       u64 val;
>> @@ -71,7 +70,7 @@ void __hyp_text __timer_restore_state(struct kvm_vcpu *vcpu)
>>       }
>>
>>       if (timer->enabled) {
>> -             write_sysreg(kvm->arch.timer.cntvoff, cntvoff_el2);
>> +             write_sysreg(vtimer->cntvoff, cntvoff_el2);
>>               write_sysreg_el0(vtimer->cnt_cval, cntv_cval);
>>               isb();
>>               write_sysreg_el0(vtimer->cnt_ctl, cntv_ctl);
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny.
>




[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux