| Message ID | 1380377113-15183-1-git-send-email-christoffer.dall@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Christoffer, On 28/09/13 15:05, Christoffer Dall wrote: > Initialize the cntvoff at vcpu_init time, not before running the VCPUs > at the first time because that will overwrite any potentially restored > values from user space. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > virt/kvm/arm/arch_timer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index 8168437..2f6b5fe 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -181,6 +181,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) > INIT_WORK(&timer->expired, kvm_timer_inject_irq_work); > hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > timer->timer.function = kvm_timer_expire; > + vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read(); > } > > static void kvm_timer_init_interrupt(void *info) > @@ -282,7 +283,6 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu) > int kvm_timer_init(struct kvm *kvm) > { > if (timecounter && wqueue) { > - kvm->arch.timer.cntvoff = kvm_phys_timer_read(); > kvm->arch.timer.enabled = 1; > } > > I'm not exactly fond of this change. CNTVOFF is a per-VM property. Here, we end-up recomputing it as we initialize VCPUs, which is very counter-intuitive. If anything, I'd move the call of kvm_timer_init out of kvm_vgic_init into kvm_arch_init_vm. What do you think? M.
On Mon, Sep 30, 2013 at 08:41:06AM +0100, Marc Zyngier wrote: > Hi Christoffer, > > On 28/09/13 15:05, Christoffer Dall wrote: > > Initialize the cntvoff at vcpu_init time, not before running the VCPUs > > at the first time because that will overwrite any potentially restored > > values from user space. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > virt/kvm/arm/arch_timer.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > > index 8168437..2f6b5fe 100644 > > --- a/virt/kvm/arm/arch_timer.c > > +++ b/virt/kvm/arm/arch_timer.c > > @@ -181,6 +181,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) > > INIT_WORK(&timer->expired, kvm_timer_inject_irq_work); > > hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > > timer->timer.function = kvm_timer_expire; > > + vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read(); > > } > > > > static void kvm_timer_init_interrupt(void *info) > > @@ -282,7 +283,6 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu) > > int kvm_timer_init(struct kvm *kvm) > > { > > if (timecounter && wqueue) { > > - kvm->arch.timer.cntvoff = kvm_phys_timer_read(); > > kvm->arch.timer.enabled = 1; > > } > > > > > > I'm not exactly fond of this change. CNTVOFF is a per-VM property. Here, > we end-up recomputing it as we initialize VCPUs, which is very > counter-intuitive. > > If anything, I'd move the call of kvm_timer_init out of kvm_vgic_init > into kvm_arch_init_vm. > > What do you think? > I think as for the anything part, we need to move it, otherwise migration won't work. Calling this from kvm_arch_init_vm sounds much cleaner actually, so I'll do that. Thanks, -Christoffer
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 8168437..2f6b5fe 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -181,6 +181,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu) INIT_WORK(&timer->expired, kvm_timer_inject_irq_work); hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); timer->timer.function = kvm_timer_expire; + vcpu->kvm->arch.timer.cntvoff = kvm_phys_timer_read(); } static void kvm_timer_init_interrupt(void *info) @@ -282,7 +283,6 @@ void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu) int kvm_timer_init(struct kvm *kvm) { if (timecounter && wqueue) { - kvm->arch.timer.cntvoff = kvm_phys_timer_read(); kvm->arch.timer.enabled = 1; }
Initialize the cntvoff at vcpu_init time, not before running the VCPUs at the first time because that will overwrite any potentially restored values from user space. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- virt/kvm/arm/arch_timer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)