> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Saturday, July 21, 2012 2:59 AM
> To: Bhushan Bharat-R65777
> Cc: kvm-...@vger.kernel.org; kvm@vger.kernel.org; ag...@suse.de; Bhushan
> Bharat-
> R65777
> Subject: Re: [PATCH 2/2 v5] KVM: PPC: booke: Add watchdog emulation
>
> On 07/20/2012 12:00 AM, Bharat Bhushan wrote:
> > This patch adds the watchdog emulation in KVM. The watchdog emulation
> > is enabled by KVM_ENABLE_CAP(KVM_CAP_PPC_WDT) ioctl.
> > The kernel timer are used for watchdog emulation and emulates h/w
> > watchdog state machine. On watchdog timer expiry, it exit to QEMU if
> > TCR.WRC is non ZERO. QEMU can reset/shutdown etc depending upon how it
> > is configured.
> >
> > Signed-off-by: Liu Yu <yu....@freescale.com>
> > Signed-off-by: Scott Wood <scottw...@freescale.com>
> > Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> > [bharat.bhus...@freescale.com: reworked patch]
>
> Typically the [] note goes immediately before your signoff (but after the
> others).
ok
>
> > +static void arm_next_watchdog(struct kvm_vcpu *vcpu) {
> > + unsigned long nr_jiffies;
> > +
> > + spin_lock(&vcpu->arch.wdt_lock);
> > + nr_jiffies = watchdog_next_timeout(vcpu);
> > + /*
> > + * If the number of jiffies of watchdog timer >= NEXT_TIMER_MAX_DELTA
> > + * then do not run the watchdog timer as this can break timer APIs.
> > + */
> > + if (nr_jiffies < NEXT_TIMER_MAX_DELTA)
> > + mod_timer(&vcpu->arch.wdt_timer, jiffies + nr_jiffies);
> > + else
> > + del_timer(&vcpu->arch.wdt_timer);
> > + spin_unlock(&vcpu->arch.wdt_lock);
> > +}
>
> This needs to be an irqsave lock.
Ok, I want to understood why irqsave lock should be used here?
irqsave_lock is used when the critical section within lock can be referenced
from interrupt context also. What part of critical section above can get
affected from local irq? Is it jiffies here?
>
> > @@ -386,13 +387,23 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> > #ifdef CONFIG_KVM_EXIT_TIMING
> > mutex_init(&vcpu->arch.exit_timing_lock);
> > #endif
> > -
> > +#ifdef CONFIG_BOOKE
> > + spin_lock_init(&vcpu->arch.wdt_lock);
> > + /* setup watchdog timer once */
> > + setup_timer(&vcpu->arch.wdt_timer, kvmppc_watchdog_func,
> > + (unsigned long)vcpu);
> > +#endif
> > return 0;
> > }
>
> Can you do this in kvmppc_booke_init()?
Ok, so the *_uninit part of code also needed to be moved in respective function.
>
> >
> > void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {
> > kvmppc_mmu_destroy(vcpu);
> > +#ifdef CONFIG_BOOKE
> > + spin_lock(&vcpu->arch.wdt_lock);
> > + del_timer(&vcpu->arch.wdt_timer);
> > + spin_unlock(&vcpu->arch.wdt_lock);
> > +#endif
> > }
>
> Don't acquire the lock here, but use del_timer_sync().
Ok.
Thanks
-Bharat
>
> -Scott
